Refactor: Split IOURequestStepScan/index.native.tsx into hooks and components#88344
Refactor: Split IOURequestStepScan/index.native.tsx into hooks and components#88344
Conversation
…omponents Extract camera init telemetry into useCameraInitTelemetry hook, photo capture orchestration into useCapturePhoto hook, and split JSX into CameraPermissionPrompt, CameraViewport, and ScannerControlsBar components. Shrinks the main component from 626 to 350 lines. Co-authored-by: truph01 <truph01@users.noreply.github.com>
Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
Fixed the failing ESLint check: replaced deprecated |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot Mark this PR as ready for review |
|
✅ This PR has been marked as ready for review. |
melvin-bot
Bot
requested review from
joekaufmanexpensify and
truph01
and removed request for
a team
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
| cancelSpan(CONST.TELEMETRY.SPAN_SHUTTER_TO_CONFIRMATION); | ||
| }, [isMultiScanEnabled]); | ||
|
|
||
| const capturePhoto = useCallback(() => { |
There was a problem hiding this comment.
Yes! Removed both useCallback wrappers (capturePhoto and maybeCancelShutterSpan). Neither needs a stable reference — ScannerControlsBar isn't memoized and neither function appears in any other hook's dependency array.
Neither capturePhoto nor maybeCancelShutterSpan need stable references since ScannerControlsBar is not memoized and neither function is used in any dependency array of other hooks. Co-authored-by: truph01 <truph01@users.noreply.github.com>
ScannerControlsBar is only rendered when cameraPermissionStatus is GRANTED, so the permission check in the disabled prop is redundant. Simplified to just !hasFlash and removed the unused cameraPermissionStatus prop. Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
The only failing checks are the PR Author Checklist and PR Reviewer Checklist — these fail because not all checklist items in the PR body are checked off yet. This is expected for a PR still in progress. All substantive CI checks (ESLint, TypeScript, tests, builds, prettier, storybook, perf-tests, etc.) are passing. No code fix is needed. |
Co-authored-by: truph01 <170341821+truph01@users.noreply.github.com> Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
Reverted |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
@MelvinBot Please check these 4 comments: #88344 (comment), #88344 (comment), #88344 (comment) and #88344 (comment). |
CameraViewport and ScannerControlsBar now call useMemoizedLazyExpensifyIcons and useMemoizedLazyIllustrations internally instead of receiving icons as props from the parent. This removes 6 prop-drilling entries (boltIcon, galleryIcon, receiptMultipleIcon, boltSlashIcon, shutterIllustration from ScannerControlsBar; boltIcon from CameraViewport) and lets each component own its asset loading. Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
Addressed all 4 review comments in 623abcd: CLEAN-REACT-PATTERNS-2 (own behavior):
CLEAN-REACT-PATTERNS-1 (composition): The icon prop-drilling was the main contributor to the high prop count. The remaining behavioral props ( |
|
@MelvinBot Could you merge main? |
|
Merged main into this branch. ✅ |
|
⏱️ Ran out of time before I could finish. This usually means the task was too complex or I got stuck exploring. Please try again with a more specific request or post in #melvinbot-ai. |
|
@MelvinBot Fix conflicts |
Resolve conflict in index.native.tsx by keeping the hook call and updating useCameraInitTelemetry to include the new entry-to-scan telemetry spans added on main. Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
Conflicts resolved. Merged |
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@mountiny All yours! |
|
🚧 @mountiny 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, and Web. Happy testing! 🧪🧪
|
|
✋ 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.3.69-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. This is a code refactoring that splits |
4 participants
Explanation of Change
IOURequestStepScan/index.native.tsxhad grown to ~626 lines, mixing camera UI with business logic. This PR splits it into focused hooks and components:useCameraInitTelemetry— owns the camera init telemetry span refs and the threeuseEffectblocks that manage CAMERA_INIT / OPEN_CREATE_EXPENSE spans, plus thehandleCameraInitializedcallbackuseCapturePhoto— encapsulates thecapturePhotofunction: permission guard, telemetry spans, photo capture, multi-scan vs. single-scan branching, editing vs. new-receipt branching, optimistic transaction creation, and Onyx receipt mergesCameraPermissionPrompt— the permission-denied view with hand icon + text + continue buttonCameraViewport—GestureDetector+NavigationAwareCamera+ animated focus indicator + blink overlay + flash buttonScannerControlsBar—AttachmentPicker+ shutter button + multi-scan/flash toggleThe main component is now a thin shell (~350 lines, down from 626) that composes these pieces.
Fixed Issues
$ #88197
Tests
Offline tests
// TODO: The human co-author must fill out the offline tests
QA Steps
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./** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari