Skip to content

fix: upsert attachmemts only when attachment exist and handle FileList for react native #1539

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
May 23, 2025
+96 −25
Merged

fix: upsert attachmemts only when attachment exist and handle FileList for react native #1539

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0d8b005
fix: upsert attachmemts only when attachment exist and handle FileLis…
khushal87 May 14, 2025
f72269c
fix: use getAttachmentIndex instead
khushal87 May 14, 2025
c1e6f54
fix: use getAttachmentIndex instead
khushal87 May 14, 2025
fb9a978
fix: improve the code as per suggestion
khushal87 May 19, 2025
63330ea
test: add updateAttachment tests
khushal87 May 19, 2025
2c61538
chore: resolve conflicts from master
khushal87 May 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions src/messageComposer/attachmentManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,11 @@ export class AttachmentManager {
},
};

this.upsertAttachments([failedAttachment]);
const isAttachmentPresent =
Copy link
Contributor

Choose a reason for hiding this comment

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

@khushal87 we could prevent duplication of these lines by adding a new method updateAttachmet to AttachmentManager. A proposal of a refactor:

private prepareAttachmentUpdate = (attachmentToUpdate: LocalAttachment) => {
    const stateAttachments = this.attachments;
    const attachments = [...this.attachments];
    const attachmentIndex = this.getAttachmentIndex(attachmentToUpdate.localMetadata.id);

    if (attachmentIndex === -1) return null;

    const merged = mergeWithDiff<LocalAttachment>(
      stateAttachments[attachmentIndex] ?? {},
      attachmentToUpdate,
    );
    const updatesOnMerge = merged.diff && Object.keys(merged.diff.children).length;
    
    if (updatesOnMerge) {
      const localAttachment = ensureIsLocalAttachment(merged.result);
      if (localAttachment) {
        attachments.splice(attachmentIndex, 1, localAttachment);
        return attachments;
      }
    }
    return null;
  };

  updateAttachment = (attachmentToUpdate: LocalAttachment) => {
    const updatedAttachments = this.prepareAttachmentUpdate(attachmentToUpdate);
    if (updatedAttachments) {
      this.state.partialNext({ attachments: updatedAttachments });
    }
  };

  upsertAttachments = (attachmentsToUpsert: LocalAttachment[]) => {
    if (!attachmentsToUpsert.length) return;
    
    let attachments = [...this.attachments];
    let hasUpdates = false;

    attachmentsToUpsert.forEach((attachment) => {
      const updatedAttachments = this.prepareAttachmentUpdate(attachment);
      if (updatedAttachments) {
        attachments = updatedAttachments;
        hasUpdates = true;
      } else {
        const localAttachment = ensureIsLocalAttachment(attachment);
        if (localAttachment) {
          attachments.push(localAttachment);
          hasUpdates = true;
        }
      }
    });

    if (hasUpdates) {
      this.state.partialNext({ attachments });
    }
  };

the in uploadAttachment you would only do this.updateAttachment(failedAttachment). WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this can do the trick as well. My bad I was testing upsertAttachments and thought this would not work but using the updateAttachments can be useful here and avoid duplication.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have pushed the code and added a few relevant tests. Let me know if its fine to merge it. Thanks 😄

this.getAttachmentIndex(failedAttachment.localMetadata.id) !== -1;
if (isAttachmentPresent) {
this.upsertAttachments([failedAttachment]);
}
return failedAttachment;
}

Expand Down Expand Up @@ -482,7 +486,12 @@ export class AttachmentManager {
(uploadedAttachment as LocalNotImageAttachment).thumb_url = response.thumb_url;
}

this.upsertAttachments([uploadedAttachment]);
const isAttachmentPresent =
this.getAttachmentIndex(uploadedAttachment.localMetadata.id) !== -1;

if (isAttachmentPresent) {
this.upsertAttachments([uploadedAttachment]);
}

return uploadedAttachment;
};
Expand Down
2 changes: 1 addition & 1 deletion src/messageComposer/fileUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ export const isFileList = (obj: unknown): obj is FileList => {
if (typeof obj !== 'object') return false;

return (
(obj as object) instanceof FileList ||
(typeof FileList !== 'undefined' && (obj as object) instanceof FileList) ||
('item' in obj && 'length' in obj && !Array.isArray(obj))
);
};
Expand Down
Loading