-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Hover-to-view for receipt thumbnails v2 #68276
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
Hover-to-view for receipt thumbnails v2 #68276
Conversation
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
No, these are all old receipts from July |
|
It seems to happen to photo receipts taken from Scan on mobile |
|
Okay, was able to repro with mobile scan, looking into it |
|
🚧 @shawnborton has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
Otherwise I think the behavior is feeling pretty good. Let's get some more testing eyes on this though! |
…-hover-receipt-v2
|
Hi, can you run test build again? I've merged newest main, to see if it resolves the issue |
|
🚧 @shawnborton has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
@mountiny I think you might have been assigned this issue as the internal engineer, do you mind taking a look? Basically when I hover over a PDF receipt, the thumbnail just loads blank. |
|
hmm,weird, I also was able to reproduce it on my account, also reproducible on dev environment I will look into it now |
|
update, found what the underlying issue is snapshot with the sama transaction: when you look at source in both of them, in openReport our image has .pdf file extension and in snapshot it has .jpg which we can normally open another thing is that in ereceipts in inbox tab the expensify card shows up nicely: however in reports tab some weird thing is showing up: it's also soemthing that we should look at edit: in the case with ereceipts we also get data that has invalid cardName returned from snapshot |
|
Agree - let's try to investigate this soon, I know we want to get this launched as soon as possible. |
|
Does everything else look good to you? If so I can get this pr out of the draft so the reviewer check, if other than the bugs above, everything works as intended and can be merged after they are resolved? |
|
Yup, let's get it into review! |
|
On my end on both ad-hoc and local build everything works well |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppUnable to build android Android: mWeb ChromeScreen.Recording.2025-08-25.at.8.13.39.AM.moviOS: HybridAppScreen.Recording.2025-08-25.at.8.08.07.AM.moviOS: mWeb SafariScreen.Recording.2025-08-25.at.7.30.10.AM.movMacOS: Chrome / SafariScreen.Recording.2025-08-25.at.7.19.02.AM.movMacOS: DesktopScreen.Recording.2025-08-25.at.7.26.49.AM.mov |
mountiny
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just small nitpick. Michal is ooo today and I think we can move it ahead now so we get the PR to staging later today
| </Animated.View>, | ||
| document.body, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Display name missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall I create a pr with it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add it to some follow up in future, no need to do it now. Also in case we need to revert it might cause more problems
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.99-0 🚀
|
|
I suspect that this PR is the cause of this blocker. |
|
@shawnborton I noted above, these issues are quite common with adhoc builds and @borys3kk or @allroundexperts could not reproduce when testing locally. That is why we moved ahead since there was not much to do if they could not reproduce. Screen.Recording.2025-08-27.at.00.53.20.movI cannot reproduce the issue you have in staging either. Can you please provide more details about the bug? There are clearly some specific conditions. Can you see any pattern between what receipt works and what does not? All that could help some since as mentioned before - nobody could reproduce |
|
@shawnborton I have also not seen any issue/ blocker raised for this, so that makes me think it is really something specific. I asked QA if they can reproduce here in Slack here https://expensify.slack.com/archives/C9YU7BX5M/p1756249418021819 to get more eyes on this |
|
Unsure what the step forward for this one here is - should we revert it if we're seeing some staging issues? |
|
I can't reproduce the issues mentioned by Shawn and Danny as Vit said |
|
Issue is not reproducible on Windows 11 / Chrome in v9.1.99-5 bandicam.2025-08-27.10-01-25-304.mp4 |
|
I can't reproduce the issue I was having either anymore. I wonder if it was just some kind of weird caching/loading issue I was experiencing that one time.
I would tend to agree with this. I don't think this requires a revert, as the feature is mostly super solid, but we should investigate the best we can and when Shawn's back we can see if he can still reproduce the bug he's experiencing. |
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.1.99-11 🚀
|
| let previewSource = transactionItem?.receipt?.source ?? ''; | ||
|
|
||
| if (source && typeof source === 'string') { | ||
| if (source) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coming from #69278, removing the typeof check resulted in a crash when creating a distance expense. This is because we on native the source is set to a number for optimistically created expenses.
| isEReceipt={isEReceipt} | ||
| transactionID={transactionItem.transactionID} | ||
| shouldUseThumbnailImage={!transactionItem?.receipt?.source} | ||
| shouldUseThumbnailImage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coming from #70651, we should handle the loading style for expenses that have receipt here. More details: #70651 (comment)
| /> | ||
| </View> | ||
| ) : ( | ||
| <EReceiptWithSizeCalculation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coming from this issue #72414 , we are handling image receipts and distance eReceipts here, so per diem was falling back to EReceiptWithSizeCalculation with the card-style view instead of using its dedicated per diem PerDiemEReceipt view. That caused the inconsistency, and we’ve fixed it here: #73911














Explanation of Change
Added custom component responsible for displaying preview of receipt in new 'window'.
Fixed Issues
$ #64195
PROPOSAL:
Tests
Testcase 1 - image preview:
Testcase 2 - eReceipt preview:
Testcase 3 - distanceEReceipt preview:
In test cases 1 and 3 the height of the preview should adjust to the height of the content (regular eReceipt has const height), If the image or distanceEReceipt is too high, the beginning should be visible like this:

Offline tests
N/A
QA Steps
Same as tests
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
Screen.Recording.2025-07-01.at.18.34.06.mov
iOS: mWeb Safari
Screen.Recording.2025-07-01.at.18.39.43.mov
MacOS: Chrome / Safari
Screen.Recording.2025-07-01.at.18.40.43.mov
MacOS: Desktop
Screen.Recording.2025-07-01.at.18.43.37.mov