Conversation
Replaces the external system camera intent with an in-app VisionCamera modal for chat attachment photos (re-land of PR 87681). Routes the camera launch through onModalHide so the camera modal only presents after the picker popover has fully dismissed, fixing the iOS modal-presentation race that caused the camera to not open / the app to load infinitely. Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
Move the cameraRef.current access into a module-level helper so React Compiler does not flag ref access during render. OXC previously bailed on the whole file (no memoization on web) while Babel still memoized it, producing a divergence. Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
|
Fixed the failing React Compiler Compliance check and pushed to the branch. Root cause: Fix: Moved the I also removed the now-stale Verification
The other failing check ( |
|
All the code checks are green now — React Compiler Compliance, ESLint, Oxfmt, typecheck, and every unit test pass after my earlier fix. The only remaining failing check is PR Author Checklist, which is a human checklist, not a code failure. There's nothing for me to fix here — |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6bd1224c6f
ℹ️ 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".
|
There's no code-related check failure to fix here — every automated CI check is passing (typecheck, ESLint, Oxfmt, React Compiler, unit tests, builds, and perf tests all green). The only two failing checks are:
These aren't code failures. They fail because the checklist boxes in the PR body aren't ticked — those items attest to human actions (manual testing on all platforms, no console errors, reviewer sign-off, etc.), so they can only be completed by a person, not by pushing a commit. To clear them:
I intentionally did not auto-tick these boxes — doing so would falsely attest that manual/on-device testing was performed. |
|
Nothing has changed since my previous comment — there's still no code-related check failure to fix. Every automated CI check is passing. The only two failing checks are PR Author Checklist / checklist and PR Reviewer Checklist / checklist. These fail because the checklist boxes in the PR body aren't ticked — those items attest to human actions (manual/on-device testing, no console errors, reviewer sign-off, filling in |
Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
|
The The two type errors
What you'll need to do
Why I didn't push a fix
The other two failing checks ( |
|
@MelvinBot Merge the latest main |
|
Fixed the failing TypeScript Checks / typecheck. The PR was green except for the two ignore-listed checklist checks and typecheck, which broke once the branch was merged against the latest
Verified locally after the merge: The remaining red checks are only PR Reviewer Checklist / checklist and Check independent approval, which are human/review steps, not code failures. |
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
There's nothing to fix in code here. The Author/Reviewer checklist checks now pass, and the remaining in-progress checks (typecheck, perf tests) aren't failing. The only failing check is Verify peer review / Check independent approval. This isn't a code failure — it's a process gate that stays red until the PR gets an independent approving review from an eligible reviewer. It clears itself once a reviewer approves; there's no commit I can push to satisfy it. |
|
The branch is already up to date with the latest
|
|
🚧 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.
|
@MelvinBot Add testing steps in the description |
|
🤖 Added testing steps to the PR description — filled in the Tests, Offline tests, and QA Steps sections. Since this is an Android/iOS Native-only change (VisionCamera is native — there's no in-app camera on web/desktop), I based the steps on the actual flow in the diff:
Please review and adjust the wording as needed before checking the boxes. |
|
@MelvinBot Let's stop passing passing |
|
Agree — we should drop Per VisionCamera's Three reasons this is the right call:
Concretely: remove Want me to push that change? Reply |
|
@MelvinBot make that change |
… folder Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
@ShridharGoel Ah okay, please skip this comment. |
|
@MelvinBot Address the other review comments except #96458 (comment) |
…reportable camera logs Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
|
Addressed all 8 remaining review threads and skipped the Code changes (4 files, +49/-41)Reused the existing permission logic — Switched to the Made both camera errors actually reach Sentry. Swapping
Both now go through Reworded the Resolved without code changes (2)Both were already satisfied by Skipped
Verification
Still native-only and unverified on-device: the iOS items from the PR description (#90119 blurry viewfinder, #90236 theme in camera mode) are untouched by this round. |
|
@MelvinBot there are conflicts |
Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com> # Conflicts: # src/libs/telemetry/ReceiptObservability.ts
|
Conflicts resolved — merged Only one file actually conflicted:
Dropping either side would have broken something, so the resolution is purely additive. I placed VerificationI confirmed the two sides were genuinely disjoint before combining them, by diffing the symbol tables of each parent:
After resolving, all four symbols are present with their usages intact, and no conflict markers remain.
The merge commit has both parents (
|
|
@huult Can you check now? |
|
@ShridharGoel Could you re-test to make sure it’s still working after the update? |
|
@ShridharGoel Please tag me when the retest is complete. |
|
@ShridharGoel Have you finished retesting this PR? Can I review it again? |
|
Not yet. @MelvinBot Can you test this? |
|
🤖 Web test steps verification results: I can only test on web in this run, and this PR's
What this does not cover: every step that actually exercises this PR — the in-app camera opening, capture, flip, flash, close, the view run · no recording available |
|
@MelvinBot Test on native Android |
|
🤖 Android test steps verification results: ❌ fail Opened the in-app VisionCamera modal from a chat and captured one photo successfully, but a second Take-photo cycle left the camera modal permanently frozen (0 fps, all buttons dead) — blocking flash-toggle and permission-denied verification and failing the modal-race check. The one successful capture also skipped the attachment preview/confirmation screen, sending directly to chat.
The blocker to look at first is step 12. On the second capture cycle the native camera session closes (CLOSING → CLOSED in logcat) but the JS camera view never unmounts, leaving a dead modal that nothing dismisses. That is the same shape as the Two caveats on the other failures: step 8 is an emulator limitation (single camera device), not a code problem, and the step 13 comment-post error may be environmental — both are worth a second look on a physical device. Steps 9 and 11 are untested, not failing, since the freeze blocked them. view run · no recording available |
|
@mountiny Can you help with triggering an Android build here? |
|
triggered |
|
🚧 robertjchen 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! 🧪🧪
|
|
@huult Tested Screen.Recording.2026-09-16.at.1.10.26.AM.mov |
|
@ShridharGoel conflicts |
|
@MelvinBot Fix conflicts |
@ShridharGoel Do you still need this? |
Resolves a conflict in config/eslint/eslint.seatbelt.tsv: main added the useMoneyReportHeaderStatusBar entry while this branch removed the useNativeCamera react-hooks/refs entry (the refactor here eliminated that violation). Kept both changes.
|
Conflicts are resolved and pushed — the PR is There was one conflicted file:
I kept both changes: VerificationRan against the merged tree:
On the React Compiler check: running it in strict The merge brought in a Note that |
|
That Android run already happened — results are in my earlier comment from 09-15, against Whether it's still needed is So a re-run would exercise the same code as both that run and If the open question is specifically whether the step-12 freeze reproduces, a fresh emulator run wouldn't add much — that run already flagged emulator-specific caveats (single camera device, synthetic feed). A physical device would be the more informative check. How I verified the merge left the camera path alone
Note the GitHub compare API caps its file list at 300, so I diffed the two parent commits locally against a full clone to get all 551. |
robertjchen
left a comment
robertjchen
left a comment
There was a problem hiding this comment.
thanks, let's ship it! 
8 participants
Explanation of Change
Slack reference: https://expensify.slack.com/archives/C05LX9D6E07/p1772818699627849
This is a fresh re-land of the in-app VisionCamera flow for chat attachment photos (originally PR #86981, re-implemented in PR #87681, which was reverted). It replaces the external system camera intent with an in-app VisionCamera modal so Expensify stays in the foreground during capture — the original crash (#84018) was the OS reclaiming the backgrounded Expensify process while the system camera was open on Android 16.
Deploy blockers addressed in this re-land
#90047(iOS: in-app camera does not open for avatar) /#90042(iOS: app loads infinitely when taking a photo with camera permission denied). Root cause: an iOS modal-presentation race. The previous version calledclose()on the picker popover and then immediately opened the cameraModal, but iOS silently drops a modal that is presented while another modal is still dismissing — so the camera never appeared (leaving the caller hanging). Fixed by deferring the camera launch toonModalHide(so it only presents after the popover has fully dismissed), reusing the exact pattern the gallery/document flow already relies on for the same iOS quirk.Not verified locally — needs on-device iOS testing before this is marked ready
#90119(iOS: viewfinder blurry/grainy) and#90236(iOS: app theme does not change while in camera mode) are iOS-runtime rendering behaviors that cannot be reproduced or validated without an iOS device. They are called out here so the reviewer verifies them on a device; if either still reproduces, they likely need a VisionCamera format/preview tweak (#90119) and a change away from the RNModalfor theme propagation (#90236) respectively.Excluded as unrelated
#93006(native-share crash) was auto-linked to PR fix: use in-app VisionCamera for chat attachment photos #87681, but its own investigation attributes it to areact-native-blob-util/react-native-svgdependency bump in PR chore: upgradereact-native-blob-utilto 0.24.9 #92509 — it is not caused by this feature, so nothing here addresses it.Local checks run and passing:
typecheck-tsgo,lint-changed, andoxfmt. React-compiler compliance will be validated by CI. All manual/on-device testing is left for the human co-author.Fixed Issues
$ #84018
PROPOSAL: #84018 (comment)
Tests
#90042): Deny camera permission (or disable it in system settings), then tap Take photo again. Verify the in-app permission prompt/screen is shown and the app does not hang on an infinite loading state.#90047): Repeat step 3 several times in quick succession and verify the camera opens reliably every time (it should never silently fail to appear).Offline tests
The in-app camera capture is a purely local operation and works fully offline:
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.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