Expensify
Attachment - When opening the last one, the first added Public Image is opened
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: 9.0.72-0 Reproducible in staging?: Yes Reproducible in production?: Yes If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Yes, reproducible on both If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5309350 Email or phone of affected tester (no customers): [email protected] Issue reported by: Applause Internal Team
Action Performed:
- Open HybridApp
- Log in to the HT account
- Go to any conversation
- Send the following attachments:
A. Image 1 B. Image 2 C.  D. Image 3 E. Image 4 F.  - Open the image from the list under the letter F
- Click on the right arrow
- Note that the first public image has opened up
Expected Result:
When opening the last added Public Image, it is the last one that must be opened
Actual Result:
When opening the last added Public Image, the first added Public Image is opened instead.
Workaround:
Unknown
Platforms:
- [x] Android: Standalone
- [x] Android: HybridApp
- [x] Android: mWeb Chrome
- [x] iOS: Standalone
- [x] iOS: HybridApp
- [x] iOS: mWeb Safari
- [x] MacOS: Chrome / Safari
- [x] MacOS: Desktop
Screenshots/Videos
https://github.com/user-attachments/assets/f10caf94-a9c4-4dd0-afa3-e1e502bf6b22
Triggered auto assignment to @JmillsExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.
![melvin-bot[bot] avatar](https://avatars.githubusercontent.com/in/179547?v=4 "Published by a pub.dev verified publisher"){kind=small}
Edited by proposal-police: This proposal was edited at 2024-12-07 00:28:37 UTC.
Proposal
Please re-state the problem that we are trying to solve in this issue.
Attachment - When opening the last one, the first added Public Image is opened
What is the root cause of that problem?
When building the attachments list in a report here https://github.com/Expensify/App/blob/48b80f65e80d304965cbeb3573d7d6678fb9bd3a/src/components/Attachments/AttachmentCarousel/extractAttachments.ts#L44-L46 we are including unique list of sources so if we have two same mardown images the source is duplicate so the second one in the order will not be included in attachments so when we open it the page index will be the first one's index. BTW it is a limitation that they have postponed it to be done later when implementing image markdown, here is the discussion
What changes do you think we should make in order to solve the problem?
We need to first start uniquely identify the attachments via their reportActionID. Add reportActionID in the attachment route https://github.com/Expensify/App/blob/48b80f65e80d304965cbeb3573d7d6678fb9bd3a/src/ROUTES.ts#L324-L326
const reportActionParam = reportActionID ? `&reportActionID=${reportActionID}` : '';
return `attachment?source=${encodeURIComponent(
url,
)}&type=${type}${reportParam}${reportActionParam}${accountParam}${authTokenParam}${fileNameParam}${attachmentLinkParam}` as const;
},
pass the reportActionID argument everywhere we use this getRoute In ReportAttachments we will get the reportActionID from the route and pass it down to AttachmentModal > Attachment Carousel Remove the set as we don't need to save unique list of sources here https://github.com/Expensify/App/blob/48b80f65e80d304965cbeb3573d7d6678fb9bd3a/src/components/Attachments/AttachmentCarousel/extractAttachments.ts#L33 Change the activeSource state to activeReportActionID https://github.com/Expensify/App/blob/48b80f65e80d304965cbeb3573d7d6678fb9bd3a/src/components/Attachments/AttachmentCarousel/index.tsx#L75
const [activeReportActionID, setActiveReportActionID] = useState<string | null>(reportActionID);
https://github.com/Expensify/App/blob/48b80f65e80d304965cbeb3573d7d6678fb9bd3a/src/components/Attachments/AttachmentCarousel/index.tsx#L86
const compareImage = useCallback(
(attachment: Attachment) => attachment.reportActionID === reportActionID && (!attachmentLink || attachment.attachmentLink === attachmentLink),
[attachmentLink, reportActionID],
);
https://github.com/Expensify/App/blob/48b80f65e80d304965cbeb3573d7d6678fb9bd3a/src/components/Attachments/AttachmentCarousel/index.tsx#L178
setActiveReportActionID(item.reportActionID);
https://github.com/Expensify/App/blob/48b80f65e80d304965cbeb3573d7d6678fb9bd3a/src/components/Attachments/AttachmentCarousel/index.tsx#L207-L210
const extractItemKey = useCallback((item: Attachment) => `reportActionID-${item.reportActionID}`, []);
https://github.com/Expensify/App/blob/48b80f65e80d304965cbeb3573d7d6678fb9bd3a/src/components/Attachments/AttachmentCarousel/index.tsx#L242
isFocused={activeReportActionID === item.reportActionID}
and do similar changes in the index.native file too
Result
https://github.com/user-attachments/assets/9f7cbe7c-03d8-4248-9d86-8c9375160269
We can write a unit test for extractAttachments passing report actions with duplicate markdown image links and assert that all of the mardown image links are returned.
Or test AttachmentCarousel in such a way that when going back from the second dupe image markdown asserting the image before it is correctly rendered.
What alternative solutions did you explore? (Optional)
@JmillsExpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
Similarly, this isn't a convert/retain/migrate bug, so filing in quality.
Job added to Upwork: https://www.upwork.com/jobs/~021866768258692435381
Triggered auto assignment to Contributor-plus team member for initial proposal review - @abdulrahuman5196 (External)
@JmillsExpensify, @abdulrahuman5196 Huh... This is 4 days overdue. Who can take care of this?
BTW it is a limitation that they have postponed it to be done later when implementing image markdown, here is the discussion
Thank you for pointing it out and the RCA seems good. Hi @roryabraham / @kidroca Since you where working on original implementation and found the issue previously as well. Could you kindly let us know if we should fix the issue as part of this? Also let us know if you have some insights on the solution suggested or problems we could face if you know.
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@JmillsExpensify @abdulrahuman5196 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!
@JmillsExpensify, @abdulrahuman5196 Whoops! This issue is 2 days overdue. Let's get this updated quick!
BTW it is a limitation that they have postponed it to be done later when implementing image markdown, here is the discussion
Thank you for pointing it out and the RCA seems good. Hi @roryabraham / @kidroca Since you where working on original implementation and found the issue previously as well. Could you kindly let us know if we should fix the issue as part of this? Also let us know if you have some insights on the solution suggested or problems we could face if you know.
Gentle ping
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@JmillsExpensify, @abdulrahuman5196 Whoops! This issue is 2 days overdue. Let's get this updated quick!
@JmillsExpensify, @abdulrahuman5196 Still overdue 6 days?! Let's take care of this!
BTW it is a limitation that they have postponed it to be done later when implementing image markdown, here is the discussion
Thank you for pointing it out and the RCA seems good. Hi @roryabraham / @kidroca Since you where working on original implementation and found the issue previously as well. Could you kindly let us know if we should fix the issue as part of this? Also let us know if you have some insights on the solution suggested or problems we could face if you know.
Gentle ping
I think people are out on holidays
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@JmillsExpensify @abdulrahuman5196 this issue is now 4 weeks old, please consider:
- Finding a contributor to fix the bug
- Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
- If you have any questions, don't hesitate to start a discussion in #expensify-open-source
Thanks!
@JmillsExpensify, @abdulrahuman5196 Eep! 4 days overdue now. Issues have feelings too...
Gentle ping @roryabraham / @kidroca on this - https://github.com/Expensify/App/issues/53690#issuecomment-2546247371
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@JmillsExpensify, @abdulrahuman5196 Whoops! This issue is 2 days overdue. Let's get this updated quick!
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
This is still reproducible.
https://github.com/user-attachments/assets/a0ae6427-c2b7-4d59-9a0e-b9815e5d30f2
ok, apologies for the delay. I've been OOO on parental leave, but I'm back now. I suppose I can help out here, let me review the issue so far
