Repository navigation
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot review |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
ReviewThe capture logic and fallback look right. One issue needs a fix: the torch doesn't come back after the camera restarts, so the beta stops working for every scan after the first. 1. The torch is not re-lit when the camera becomes active again
Here is how to reproduce it: turn flash on, scan, go back from confirmation, and scan again. The flash icon shows on, but the torch is dark. The capture logs Suggested fix: pass an 2.
|
|
@marufsharifi Please 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe1d4b9dfe
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| torchRequestedAt.current = Date.now(); | ||
| camera.current?.controller?.setTorchMode('on').catch((error: unknown) => { |
There was a problem hiding this comment.
Start settling after the torch actually turns on
On Android devices where setTorchMode('on') takes most or all of the 500 ms window to complete, especially after a session restart, this starts the settle clock before the asynchronous request has lit the torch. As soon as controller.torchMode first becomes on, the elapsed time since the request may already exceed TORCH_SETTLE_MS, so a shutter press can take a snapshot before auto-exposure has had 500 ms under the new light; if the background upgrade fails or is claimed first, that underexposed snapshot is the submitted receipt. Start the settle interval when the torch is confirmed on (or after the setter fulfills), not before requesting it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed for the restart path: the settle clock now restarts when setTorchMode('on') resolves, and there’s a test for a slow torch. When flash is first turned on, the torch comes from the torchMode prop, which gives no signal for when it’s lit. I’m leaving that path as is: it needs a shutter press within about 500 ms of the torch lighting, and the photo upgrade usually replaces the snapshot.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen_Recording_20261027_183344_Expensify.Dev.mp4Android: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Good for product
mountiny
left a comment
There was a problem hiding this comment.
Thanks for this, looks good to me
|
@OlimpiaZurek can you please resolve the conflicts? |
# Conflicts: # src/pages/iou/request/step/IOURequestStepScan/components/Camera/index.native.tsx
@mountiny done |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.5.31-0 🚀
|
|
No help site update needed — the Android capture change is gated behind the |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.5.31-0 🚀
|
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.5.31-4 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
On Android, a scan with flash on takes a full photo, because a snapshot can't fire the flash. The camera meters the pre-flash first, so the photo takes about a second, and these captures make up the slow tail of
ManualShutterToConfirmation.With the new
scanFlashTorchbeta, turning flash on lights the torch instead, and the shutter takes a snapshot of the lit preview. The existing background upgrade then replaces it with a full photo, the same as for a scan without flash. This PR only changes Android single scans that run the upgrade. Multi-scan, skip-confirmation and edit-receipt keep the flash photo, and so does iOS.Local p95 on one Android phone, 15 captures each:
ManualReceiptCapturep95ManualShutterToConfirmationp95In production the Android p95 of
ManualShutterToConfirmationis 884ms while p90 is 458ms, and the flash captures account for that gap. We estimate the p95 drops to about 570ms once they go through the snapshot path.Fixed Issues
$ #104482
PROPOSAL:
Tests
Requires the
scanFlashTorchbeta. Use a paper receipt.Android:
iOS:
Offline tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari