Files - Migrate attachments in activities#17808
Conversation
Greptile OverviewGreptile SummaryThis PR migrates activity (note/task) rich-text attachments toward the new Files Field model by:
Key issues to fix before merge:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant FE as Frontend (ActivityRichTextEditor)
participant Hook as useUploadAttachmentFile
participant GQL as GraphQL API (uploadFilesFieldFile)
participant FF as FeatureFlagService
participant FFS as FilesFieldService
participant DB as DB/File storage
FE->>Hook: uploadAttachmentFile(file, target)
Hook->>FF: isFeatureEnabled(IS_FILES_FIELD_MIGRATED)
alt files-field migrated
Hook->>GQL: uploadFilesFieldFile(fieldMetadataId, file)
GQL->>FFS: uploadFile(buffer, filename, ...)
FFS->>DB: writeFile_v2 + save FileEntity
FFS-->>GQL: FilesFieldFileDTO{id,path,size,createdAt,url}
Hook-->>FE: {attachmentAbsoluteURL: url, attachmentFileId: id}
FE->>FE: urlToFileIdMap.set(url, id)
FE->>FE: Persist body enriched with attachmentFileId
else legacy
Hook->>GQL: uploadFile (legacy)
Hook-->>FE: {attachmentAbsoluteURL: fullPath}
end
Note over FE: Later reads of activity body
FE->>GQL: query note/task
GQL->>FF: isFeatureEnabled(IS_FILES_FIELD_MIGRATED)
alt migrated and attachmentFileId present
GQL->>FFS: signFileUrl(fileId, workspaceId)
GQL-->>FE: block props.url replaced with signed /files-field URL
else legacy image
GQL->>DB: sign legacy /files/attachment URL
GQL-->>FE: block props.url replaced with signed /files URL
end
|
.../upgrade-version-command/1-18/1-18-migrate-activity-rich-text-attachment-file-ids.command.ts
Show resolved
Hide resolved
Additional Comments (1)
When Context Used: Context from |
There was a problem hiding this comment.
2 issues found across 29 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-front/src/modules/activities/files/components/AttachmentRow.tsx">
<violation number="1" location="packages/twenty-front/src/modules/activities/files/components/AttachmentRow.tsx:107">
P2: Force-casting the migrated file URL to string removes the previous runtime guard, so `fileUrl` can be undefined and be used in `href`/`downloadFile`, leading to broken links or errors when `attachment.file` is empty. Keep a runtime check or fallback for the migrated case.</violation>
</file>
<file name="packages/twenty-server/src/database/commands/upgrade-version-command/1-18/1-18-migrate-activity-rich-text-attachment-file-ids.command.ts">
<violation number="1" location="packages/twenty-server/src/database/commands/upgrade-version-command/1-18/1-18-migrate-activity-rich-text-attachment-file-ids.command.ts:307">
P1: `findMatchingFileId` can throw from `getAttachmentLegacyRelativePath`/`getAttachmentNewdRelativePath` if the URL doesn't match expected patterns. This is called without try/catch in the migration loop, so a single unexpected URL format (e.g., an external URL) will crash the migration for the entire workspace. Wrap the call in a try/catch or add a guard to skip unrecognized URL formats.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
.../upgrade-version-command/1-18/1-18-migrate-activity-rich-text-attachment-file-ids.command.ts
Show resolved
Hide resolved
packages/twenty-front/src/modules/activities/files/components/AttachmentRow.tsx
Show resolved
Hide resolved
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:39202 This environment will automatically shut down when the PR is closed or after 5 hours. |
1b6a200 to
fae6b68
Compare
.../upgrade-version-command/1-18/1-18-migrate-activity-rich-text-attachment-file-ids.command.ts
Outdated
Show resolved
Hide resolved
f35385c to
f0dce0a
Compare
...mon-result-getters/handlers/field-handlers/rich-text-v2-field-query-result-getter.handler.ts
Outdated
Show resolved
Hide resolved
.../upgrade-version-command/1-18/1-18-migrate-activity-rich-text-attachment-file-ids.command.ts
Show resolved
Hide resolved
.../upgrade-version-command/1-18/1-18-migrate-activity-rich-text-attachment-file-ids.command.ts
Show resolved
Hide resolved
packages/twenty-front/src/modules/activities/components/ActivityRichTextEditor.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/activities/components/ActivityRichTextEditor.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/activities/components/ActivityRichTextEditor.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/activities/components/ActivityRichTextEditor.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/activities/components/ActivityRichTextEditor.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/activities/utils/__tests__/filterAttachmentsToRestore.test.ts
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/activities/utils/getAttachmentUrl.ts
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/activities/utils/getAttachmentUrl.ts
Outdated
Show resolved
Hide resolved
prastoin
left a comment
There was a problem hiding this comment.
Will re-review backend when you've implemented the refactor we've been discussing about !
...mon-result-getters/handlers/field-handlers/rich-text-v2-field-query-result-getter.handler.ts
Show resolved
Hide resolved
.../upgrade-version-command/1-18/1-18-migrate-activity-rich-text-attachment-file-ids.command.ts
Show resolved
Hide resolved
.../upgrade-version-command/1-18/1-18-migrate-activity-rich-text-attachment-file-ids.command.ts
Show resolved
Hide resolved
e41220f to
ec81481
Compare
...mon-result-getters/handlers/field-handlers/rich-text-v2-field-query-result-getter.handler.ts
Outdated
Show resolved
Hide resolved
.../upgrade-version-command/1-18/1-18-migrate-activity-rich-text-attachment-file-ids.command.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
2 issues found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-server/src/database/commands/upgrade-version-command/1-18/1-18-migrate-activity-rich-text-attachment-file-ids.command.ts">
<violation number="1" location="packages/twenty-server/src/database/commands/upgrade-version-command/1-18/1-18-migrate-activity-rich-text-attachment-file-ids.command.ts:212">
P2: Avoid loading full activity rows when only id/bodyV2 are needed; this increases memory and DB load for large workspaces.</violation>
<violation number="2" location="packages/twenty-server/src/database/commands/upgrade-version-command/1-18/1-18-migrate-activity-rich-text-attachment-file-ids.command.ts:351">
P2: Map keys should be normalized to the same relative path format used in findMatchingFileId; storing raw fullPath will prevent matches and can create duplicate attachments.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| `Migrating ${activityType} rich text for workspace ${workspaceId}`, | ||
| ); | ||
|
|
||
| const activities = await activityRepository.find(); |
There was a problem hiding this comment.
P2: Avoid loading full activity rows when only id/bodyV2 are needed; this increases memory and DB load for large workspaces.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/database/commands/upgrade-version-command/1-18/1-18-migrate-activity-rich-text-attachment-file-ids.command.ts, line 212:
<comment>Avoid loading full activity rows when only id/bodyV2 are needed; this increases memory and DB load for large workspaces.</comment>
<file context>
@@ -209,9 +209,7 @@ export class MigrateActivityRichTextAttachmentFileIdsCommand extends ActiveOrSus
- const activities = await activityRepository.find({
- select: ['id', 'bodyV2'],
- });
+ const activities = await activityRepository.find();
this.logger.log(
</file context>
| const activities = await activityRepository.find(); | |
| const activities = await activityRepository.find({ | |
| select: ['id', 'bodyV2'], | |
| }); |
| const fileData = attachment.file[0]; | ||
|
|
||
| if (isDefined(attachment.fullPath) && isDefined(fileData.fileId)) { | ||
| urlToFileIdMap.set(attachment.fullPath, fileData.fileId); |
There was a problem hiding this comment.
P2: Map keys should be normalized to the same relative path format used in findMatchingFileId; storing raw fullPath will prevent matches and can create duplicate attachments.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/database/commands/upgrade-version-command/1-18/1-18-migrate-activity-rich-text-attachment-file-ids.command.ts, line 351:
<comment>Map keys should be normalized to the same relative path format used in findMatchingFileId; storing raw fullPath will prevent matches and can create duplicate attachments.</comment>
<file context>
@@ -350,11 +348,7 @@ export class MigrateActivityRichTextAttachmentFileIdsCommand extends ActiveOrSus
- );
-
- urlToFileIdMap.set(normalizedPath, fileData.fileId);
+ urlToFileIdMap.set(attachment.fullPath, fileData.fileId);
}
}
</file context>
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-server/src/engine/api/common/common-result-getters/handlers/field-handlers/__tests__/rich-text-v2-field-query-result-getter.handler.spec.ts">
<violation number="1" location="packages/twenty-server/src/engine/api/common/common-result-getters/handlers/field-handlers/__tests__/rich-text-v2-field-query-result-getter.handler.spec.ts:176">
P2: The new spy uses `mockResolvedValue(false)` which persists across tests because `afterEach` only calls `jest.clearAllMocks()` (it does not restore spy implementations). This can unintentionally force the feature flag off in later tests. Prefer a one-time mock to keep the change scoped to this test.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| it('when image block has an internal attachment URL (legacy path)', async () => { | ||
| jest | ||
| .spyOn(mockFeatureFlagService, 'isFeatureEnabled') | ||
| .mockResolvedValue(false); |
There was a problem hiding this comment.
P2: The new spy uses mockResolvedValue(false) which persists across tests because afterEach only calls jest.clearAllMocks() (it does not restore spy implementations). This can unintentionally force the feature flag off in later tests. Prefer a one-time mock to keep the change scoped to this test.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/engine/api/common/common-result-getters/handlers/field-handlers/__tests__/rich-text-v2-field-query-result-getter.handler.spec.ts, line 176:
<comment>The new spy uses `mockResolvedValue(false)` which persists across tests because `afterEach` only calls `jest.clearAllMocks()` (it does not restore spy implementations). This can unintentionally force the feature flag off in later tests. Prefer a one-time mock to keep the change scoped to this test.</comment>
<file context>
@@ -170,7 +170,11 @@ describe('RichTextV2FieldQueryResultGetterHandler', () => {
+ it('when image block has an internal attachment URL (legacy path)', async () => {
+ jest
+ .spyOn(mockFeatureFlagService, 'isFeatureEnabled')
+ .mockResolvedValue(false);
+
const imageBlock = {
</file context>
| .mockResolvedValue(false); | |
| .mockResolvedValueOnce(false); |
| } | ||
|
|
||
| const pathname = parsedUrl.pathname; | ||
| const isLinkExternal = !pathname.startsWith('/files-field/'); |
There was a problem hiding this comment.
Remark: Not sure about that, why wouldn't we be looking to the domain ? I'm pretty sure there's a reason, do you have any information @charlesBochet ?
| let fileId = this.findMatchingFileId(url, urlToFileIdMap); | ||
|
|
||
| if (!isDefined(fileId) && !isDryRun) { | ||
| const createdFileId = await this.createAttachmentFromUrl( |
There was a problem hiding this comment.
remark: if an operation file during a blocknote migration we would have some standalone files, nothing critical though
|
Hey @etiennejouan! After you've done the QA of your Pull Request, you can mark it as done here. Thank you! |
1 similar comment
|
Hey @etiennejouan! After you've done the QA of your Pull Request, you can mark it as done here. Thank you! |

As attachment files have migrated from fullPath to file files field, need to migrate richText logic to fit to new attachment file handling + data migration