messaging folders custom graphql resolver #16506
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 15 files
Prompt for AI agents (all 1 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-server/src/modules/messaging/message-folder-manager/services/message-folder-sync-status.service.ts">
<violation number="1" location="packages/twenty-server/src/modules/messaging/message-folder-manager/services/message-folder-sync-status.service.ts:136">
P1: Missing `messageChannelId` constraint in the update operation could allow modification of folders from other message channels. The validation query at line 88 correctly filters by `messageChannelId`, but this constraint is not applied to the actual update, potentially allowing unauthorized cross-channel modifications.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
.../src/modules/messaging/message-folder-manager/services/message-folder-sync-status.service.ts
Show resolved
Hide resolved
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:41585 This environment will automatically shut down when the PR is closed or after 5 hours. |
| describe('when syncing a folder', () => { | ||
| it('should include the folder itself for a root folder', () => { | ||
| const inbox = createFolder('inbox', 'Inbox'); | ||
| const result = computeFolderIdsForSyncToggle('inbox', [inbox], true); |
There was a problem hiding this comment.
this is hard to understand from the outside, let's use object as parameter instead of several params
.../modules/settings/accounts/components/message-folders/utils/computeFolderIdsForSyncToggle.ts
Show resolved
Hide resolved
| const collectChildren = (id: string): string[] => { | ||
| const folder = folderById.get(id); | ||
| const children = folder?.externalId | ||
| ? allFolders.filter((f) => f.parentFolderId === folder.externalId) |
| const parents: MessageFolder[] = []; | ||
| let current = folderById.get(id); | ||
|
|
||
| while (current?.parentFolderId) { |
There was a problem hiding this comment.
not a fan of current?. => let's early return instead, this simplifies mental model
| export const computeFolderIdsForSyncToggle = ( | ||
| folderId: string, | ||
| allFolders: MessageFolder[], | ||
| isSyncing: boolean, |
.../modules/settings/accounts/components/message-folders/utils/computeFolderIdsForSyncToggle.ts
Show resolved
Hide resolved
| messageFolderIds, | ||
| isSynced, | ||
| }: UpdateMessageFoldersSyncStatusArgs) => { | ||
| const cachedFolders = messageFolderIds |
There was a problem hiding this comment.
it's great that you care about optimstic rendering
Reading about this code, I feel we should create a generic tooling like we have in useUpdateOneRecord, let's introduce useUpdateManyRecords, I believe we already have an api endpoint for that
==> so the custom resolver my not be useful :(
|
|
||
| @InputType() | ||
| export class UpdateMessageFoldersSyncStatusInput { | ||
| @IsUUID('4') |
There was a problem hiding this comment.
first time I see this '4', is it mandatory?
| transactionManager, | ||
| ); | ||
|
|
||
| if (!messageChannel) { |
| } | ||
|
|
||
| if ( | ||
| messageChannel.messageFolderImportPolicy !== |
There was a problem hiding this comment.
this logic is important, we can make it a pre-hook. A bit weird to throw WorkspaceQueryRunnerException here, we should throw messaging exceptions
| ) { | ||
| if (messageFolderIds.length > 1) { | ||
| const priorityFolder = foldersToToggle.find((folder) => { | ||
| if (!folder.name || !folder.isSynced) return false; |
There was a problem hiding this comment.
this code has too much nesting, let's make it simple (use functions and early returns)
|
|
||
| if (priorityFolder) { | ||
| const foldersToUnsync = messageFolderIds.filter( | ||
| (folderId) => folderId !== priorityFolder.id, |
This PR adds custom GraphQL resolver for updating sync status of message folders