Add primary receipt scan option on mobile and web#72236
Conversation
melvin-bot
Bot
requested review from
ZhenjaHorbach and
mountiny
and removed request for
a team
|
@ZhenjaHorbach @mountiny One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
JKobrynski
changed the title
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
| const [activePolicyID] = useOnyx(ONYXKEYS.NVP_ACTIVE_POLICY_ID, {canBeMissing: true}); | ||
| const [activePolicy] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${activePolicyID}`, {canBeMissing: true}); | ||
| const [session] = useOnyx(ONYXKEYS.SESSION, {canBeMissing: false, selector: sessionSelector}); | ||
| const [allReports] = useOnyx(ONYXKEYS.COLLECTION.REPORT, {canBeMissing: true}); |
There was a problem hiding this comment.
We need the allReports to load policyChats - I think it might be better to already have a selector that loads all the policyExpenseChats you own and then getWorkspaceChats would only have those in the useMemo
Or even better, can we put the full policyChatForActivePolicy logic into the useOnyx selector?
We should try as hard as possible not to useOnyx on a full collection
|
Minor issue 2025-10-10.15.40.54.mov |
|
Actually interesting Maybe we just need to hide this icon for small desktop screens? CC: @mountiny 2025-10-10.15.46.35.mov |
I assume we would only want the button on mobile (devices with camera, no?) @Expensify/design |
|
@shawnborton I tried reproducing that misalignment on desktop caused by odd pixels and I couldn't. Are you using some specific setup/window size? 
|
|
Are you zoomed in at all? I am just using Chrome web on a MacBook at 100% zoom. |
Oh you mentioned it's desktop so I was checking the desktop app. I just checked on web with 100% zoom and everything is fine too. |
|
🚧 @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! 🧪🧪
|
Performance Rule Violations FoundI found several performance violations in the code that need to be addressed: Rule PERF-4 violations: Objects and functions passed as props should be properly memoized to prevent unnecessary re-renders. Rule PERF-6 violations: Hook dependencies should specify individual properties instead of entire objects to create more granular dependency tracking. Please see the inline comments for specific fixes. |
|
LGTM |
. Thank you for your hard work!
HelpDot Documentation Review - Not ApplicableAssessmentThis PR (#72236) does not contain HelpDot documentation changes. The PR is focused on adding a primary receipt scan option on mobile and web platforms and includes only code, assets, and localization files. Changed Files AnalysisThe 23 changed files consist of:
RecommendationNo HelpDot documentation review is needed for this PR. If documentation updates are required for this new feature, they should be submitted as a separate PR to the docs/ directory following the HelpDot documentation standards. This assessment was automatically generated by the HelpDot Documentation Review system. |
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Some minor comments to address in the follow ups
| import {Camera} from './Icon/Expensicons'; | ||
| import {PressableWithoutFeedback} from './Pressable'; | ||
|
|
||
| const sessionSelector = (session: OnyxEntry<OnyxTypes.Session>) => ({email: session?.email, accountID: session?.accountID}); |
There was a problem hiding this comment.
Does this not exist in the shared selectors? I would think it must exist
| selector: (reports) => { | ||
| if (isEmptyObject(activePolicy) || !activePolicy?.isPolicyExpenseChatEnabled) { | ||
| return undefined; | ||
| } | ||
| const policyChatsForActivePolicy = getWorkspaceChats(activePolicyID, [session?.accountID ?? CONST.DEFAULT_NUMBER_ID], reports); | ||
| return policyChatsForActivePolicy.at(0); | ||
| }, |
There was a problem hiding this comment.
The selectors should not be inline, can you refactor this to use stable reference to selector?
| style={[ | ||
| styles.navigationTabBarFABItem, | ||
| styles.ph0, | ||
| // Prevent text selection on touch devices (e.g. on long press) |
There was a problem hiding this comment.
| // Prevent text selection on touch devices (e.g. on long press) | |
| // Prevent text selection on touch devices (e.g. on long press) |
| const shouldDisplayLHB = !shouldUseNarrowLayout; | ||
|
|
||
| const platform = getPlatform(true); | ||
| // We want to display the floating camera button on mobile devices (both web and native) |
There was a problem hiding this comment.
| // We want to display the floating camera button on mobile devices (both web and native) | |
| // We want to display the floating camera button on mobile devices (both web and native) |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
@dubielzyk-expensify do you need any follow-ups on this? Let me know. The comments took off after that, so I'm not sure where we landed. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.2.31-0 🚀
|
|
@jamesdeanexpensify looks like it merged already. Let's see when it hits prod and we can change from there 👍 |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.2.31-2 🚀
|
12 participants
Explanation of Change
Fixed Issues
$ #72030
PROPOSAL: N/A
Tests
Offline tests
N/A
QA Steps
Same as Tests section above.
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))npm run compress-svg)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-compressed.webm
Android: mWeb Chrome
chrome-compressed.webm
iOS: Native
ios-compressed.mp4
iOS: mWeb Safari
safari-compressed.mp4
MacOS: Chrome / Safari
web-compressed.mov
MacOS: Desktop
desktop-compressed.mov