Add receipt capture-to-upload observability logging#94950
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@codex review |
|
@mkhutornyi 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: 88b51f71d2
ℹ️ 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".
| isMultiScanEnabled, | ||
| transactions, | ||
| draftTransactionIDsToCleanUp, | ||
| captureSource = 'file', |
There was a problem hiding this comment.
Preserve the real capture source
When this helper is used from the scan camera's onCapture path or the gallery/file picker's onPicked path, none of the call sites pass captureSource, so every new receipt is logged as captureSource: 'file'. That makes camera and gallery captures indistinguishable/mislabeled in the new [Receipt] captured telemetry, which undermines the capture-source analysis this instrumentation is meant to support; please pass the source from the camera/picker call sites instead of defaulting all captures to file.
Useful? React with 👍 / 👎.
| WRITE_COMMANDS.SPLIT_BILL, | ||
| WRITE_COMMANDS.SPLIT_BILL_AND_OPEN_REPORT, | ||
| WRITE_COMMANDS.START_SPLIT_BILL, | ||
| WRITE_COMMANDS.COMPLETE_SPLIT_BILL, |
There was a problem hiding this comment.
Exclude receiptless split commands
For offline/manual split bills using SplitBill, SplitBillAndOpenReport, or CompleteSplitBill, the request params written by Split.ts don't include a receipt, but adding these commands to the receipt-bearing set makes push() and queue snapshots emit [Receipt] logs with no trace id for ordinary split requests. That pollutes the pending-receipt trail with non-receipt queue entries; either filter on data.receipt before logging/snapshotting or keep only commands whose params actually carry the local receipt file.
Useful? React with 👍 / 👎.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
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.
|
|
Please address codex reviews and fix conflict |
|
🤖 Code review — receipt capture-to-upload observability logging Overall this is a clean, well-documented, logging-only change with genuinely good test coverage (capture/snapshot behavior, the Sentry PII whitelist, and the end-to-end 1. ~Half the diff is unrelated formatting churn — hurts reviewability. A large share of the 2. 3. The Sentry param whitelist broadening is global, not Minor notes
Next Steps: Reply with |
|
@mkhutornyi I addressed the review feedback: Formatting churn (MelvinBot 1) - Reverted the stray oxfmt object-expansion on untouched lines back to main's style across App.ts, MoneyRequest.ts, RequestMoneyTest.ts, SequentialQueue.ts, and the 3 Scan components Unbounded timing map (MelvinBot 2) - enqueuedAtByTransactionID is now bounded by a cap (MAX_TRACKED_ENQUEUE_TIMESTAMPS = 100) in addition to the snapshot-path drain, so a long-lived session that never backgrounds/signs out can't grow it without bound Sentry whitelist scope (MelvinBot 3) - event, transactionID, and receiptTraceId are no longer in the global PARAMETERS_WHITELIST; they're now in a prefix-scoped whitelist that only forwards them for [Receipt] lines. Also dded a regression test covering this Codex P2s - both were already handled before - camera/gallery/replace paths now pass the real captureSource (no more everything-as-file), and receipt split commands are gated on data.receipt in both the enqueue and snapshot paths Conflicts are also resolved 👍 |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #92139 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
cead22
left a comment
There was a problem hiding this comment.
- Conflicts
- Please test sign out, background, and foreground cases
- With AI writing most code, we should strive to make our code comments very easy to understand. AI by default uses CAPS, ;, dashes, → , parenthesis, hyphenated words, and other words that aren't common to find in human written comments. Please update the code comments to make them easy to parse and understand by humans
|
I resolved conflicts and I updated the comments as requested. Pasting the recording for test for foreground/background and signOut: Foreground Background: BackgroundForeground.mp4signOut: SignOut.mp4Also here we have the follow up PR which shows new modal and saves the receipt in the user gallery before signing out -> #94973 |
| receipt?: {receiptTraceId?: string}; | ||
| }; | ||
| // Only log when there is a receipt at data.receipt. SplitBill nests it in the splits JSON, and SendMoney and | ||
| // friends can run without one. A row without a trace id cannot be joined to the capture log, so it is just noise. |
There was a problem hiding this comment.
...SendMoney and friends can run without one.
I think this need to be either more precise or dropped? We should rather state that some commands do not have this appended.
There was a problem hiding this comment.
I mean the wording just seem to be too loose
There was a problem hiding this comment.
Yeah lets update this but I wont block on it @adhorodyski @rinej
| } | ||
| } | ||
|
|
||
| // Save the request to the persisted queue. The in-memory update inside save() |
There was a problem hiding this comment.
not sure why we're documenting it here, this promise is not related to the observability, right?
mountiny
left a comment
There was a problem hiding this comment.
Excited to see what these observability enhancments will bring us when debugging the flows
| receipt?: {receiptTraceId?: string}; | ||
| }; | ||
| // Only log when there is a receipt at data.receipt. SplitBill nests it in the splits JSON, and SendMoney and | ||
| // friends can run without one. A row without a trace id cannot be joined to the capture log, so it is just noise. |
There was a problem hiding this comment.
Yeah lets update this but I wont block on it @adhorodyski @rinej
| /** | ||
| * Write commands whose params can carry a captured receipt. A pending request with one of these commands is a receipt | ||
| * that has not reached the server yet. Keep this in sync with the durability slice so the two features agree on which | ||
| * queued requests own a local receipt file. | ||
| */ | ||
| const RECEIPT_BEARING_COMMANDS = new Set<string>([ | ||
| WRITE_COMMANDS.REQUEST_MONEY, | ||
| WRITE_COMMANDS.TRACK_EXPENSE, | ||
| WRITE_COMMANDS.SPLIT_BILL, |
There was a problem hiding this comment.
This was already in the other PR so I assume we need to clean these up now
Add the required delegateAccountID field to the requestMoney call introduced in Expensify#94950. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@adhorodyski @mountiny |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.33-0 🚀
|
|
🤖 I reviewed the changes in this PR against Expensify's help site content under No help site changes are required, and no draft PR was created. This PR is logging/observability-only with no user-facing behavior change:
Since no docs update is needed, there is no linked help site PR to review. If you believe a user-facing behavior did change and should be documented, reply with |
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.4.33-8 🚀
Bundle Size Analysis (Sentry): |

Explanation of Change
It is based on the initial PR -> #94212
Adds structured logging so we can follow a single receipt from capture through to upload. Each receipt gets a unique trace id at capture time, and we log [Receipt] lines at three points: captured, submitted to the API, and enqueued in the network queue. We also log a snapshot of any still-pending receipts at sign-out, app backgrounding, and foregrounding.
The trace id and transaction id are also sent to Sentry as a breadcrumb and tag, so if the app crashes mid-upload the crash report shows the receipt's trail.
Fixed Issues
$ #92139
PROPOSAL:
Tests
This PR is logging-only - no user-facing behavior change.
adb logcat/Safari Web Inspector on native) and filter for[Receipt]. After step 1 confirm three lines appear:event: captured,event: submitted,event: enqueued, all carrying the samereceiptTraceId.[Receipt] queue snapshotline is emitted withtrigger: signOut.Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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
LogsReceipts.mp4
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari