Skip to content
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.

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

Fix: Single image shown multiple times #5966

Closed
wants to merge 6 commits into from

Conversation

Rohit3523
Copy link
Contributor

@Rohit3523 Rohit3523 commented Nov 7, 2024

Proposed changes

This pull request fixes the issue where if we attach multiple image URLs in a single message payload, the App only showed the last image multiple times. The root cause was in attachment caching logic. While mapping through the attachments array, we were incorrectly updating all attachment URLs to the same value. To fix this, I modified the code to compare the download URL with each attachment's image URL during the mapping process:

attachments?.map(att => ({
    ...att,
    title_link: downloadUrl === att.image_url ? uri : att.title_link,
    e2e: encryption ? 'done' : undefined
}));

Issue(s)

#5942

How to test or reproduce

To test this issue please use the following api

URL: https://********.rocket.chat/api/v1/chat.postMessage
Method: POST
Header: {
     'X-Auth-Token': '**********',
     'X-User-Id': '**********'
}
body: {
  "roomId": "",
  "attachments": [
    {
      "image_url": "https://i.pinimg.com/474x/c4/23/63/c423639b72feb5c93a14144fb50008c3.jpg" //or any random image url
    },
    {
      "image_url": "https://i.pinimg.com/474x/fc/98/6e/fc986eb80193639da37bf34cc5f6ef57.jpg" //or any random image url
    }
  ]
}

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves a current function)
  • New feature (non-breaking change which adds functionality)
  • Documentation update (if none of the other choices apply)

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works (if applicable)
  • I have added necessary documentation (if applicable)
  • Any dependent changes have been merged and published in downstream modules

@Rohit3523
Copy link
Contributor Author

Before: Screenshot_2024_1108_040111.jpg

@Rohit3523
Copy link
Contributor Author

Rohit3523 commented Nov 7, 2024

After:
Screenshot 2024-11-08 at 4 01 25 AM
Screenshot 2024-11-08 at 4 01 37 AM

(I have to attach 2 screenshots because emulator don't have scroll screenshot option)

Copy link
Member

@diegolmello diegolmello left a comment

Choose a reason for hiding this comment

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

This is awesome! Thanks for fixing it!
Is it opening AttachmentView correctly?

@Rohit3523
Copy link
Contributor Author

@diegolmello AttachmentView is showing correct image

@diegolmello
Copy link
Member

@Rohit3523 Ok. @OtavioStasiak is going to review this and we can plan it for 4.55.0 later this month.

@Rohit3523
Copy link
Contributor Author

This pull request also fixes an issue where, if we attempt to send the same images multiple times in the same channel, one of them fails to open in preview mode.

Explanation: If I send images 1 and 2 together in a single message from the API and then send the same images again, only the latest images are clickable and viewable in preview mode on the chat screen. The previous images do not open in preview mode.

@Rohit3523
Copy link
Contributor Author

I have made the changes to display image from cache

Copy link
Contributor

@OtavioStasiak OtavioStasiak left a comment

Choose a reason for hiding this comment

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

Everything works fine on both platforms, nice job!

Tested on:
iPhone 15 (IOS 17.5)
android Pixel 8 (API 35)

@Rohit3523
Copy link
Contributor Author

Thanks ^_^

@diegolmello
Copy link
Member

The rest of the work is going to be finished on #6068

@diegolmello
Copy link
Member

@Rohit3523 Send me your github email so I can add you as co-author there.
Thanks for contributing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants