[Payment due @abzokhattab] Show a page count badge on multi-page PDF receipts - #97813
[Payment due @abzokhattab] Show a page count badge on multi-page PDF receipts#97813ishpaul777 wants to merge 17 commits into
Conversation
Auth now sends the page count for receipts stored as a PDF. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The receipt thumbnail is a server-generated JPG of page 1, so when a total sits on a later page the expense looks like it disagrees with its own receipt, and nothing signals that more pages exist. The badge sits bottom-left because the receipt action buttons hold the top-right corner, and uses the same translucent dark background as the video player controls so it stays legible over any receipt content in either theme. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
Design settled on the plain Badge rather than a bespoke translucent pill with an icon, so this drops the custom styles and the page-stack icon and keeps only the positioning override. The badge also moves out of ReceiptHoverZoom's children and becomes a sibling of the zoom wrapper, alongside the receipt action buttons. Inside, hover-zoom magnified the badge along with the receipt; outside, it stays put while the receipt scales under it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Upstream stopped importing parameter types from params.ts and now types translation parameters inline, so the page count copy follows that convention and ReceiptPageCountParams is gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The receipt container takes flex1 while loading, so it stretches and an absolutely positioned badge lands at the bottom of that taller box rather than on the receipt. The receipt action buttons already wait for the load for the same reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Generated with scripts/generateTranslations.ts, which the CI workflow cannot run for a PR from a fork. French came back unchanged from the model and fell back to English, so it is translated by hand. Spanish already matched what the script produced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@abzokhattab 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] |
DebugUtils mirrors the Receipt type as a runtime field map, and both validators must enumerate every key, so adding pageCount to the type broke typecheck until they listed it too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 281aa9b36e
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const hasReceipt = hasReceiptTransactionUtils(displayedTransaction); | ||
| // The thumbnail only ever renders page 1 of a PDF, so a total sitting on a later page looks like it | ||
| // disagrees with the expense amount. Only multi-page receipts need the badge. | ||
| const receiptPageCount = displayedTransaction?.receipt?.pageCount ?? 0; |
There was a problem hiding this comment.
Clear stale page counts when replacing receipts
When a multi-page PDF is replaced, replaceReceipt() in src/libs/actions/IOU/Receipt.ts applies an Onyx MERGE whose nested receiptOptimistic object omits pageCount; therefore the previous count remains in Onyx. Since this line trusts that retained value without checking whether the new receipt is a PDF, replacing the PDF with an image—or with a PDF having fewer pages—shows the old Page 1 of N badge after loading, and an image response that omits this PDF-only field may never clear it. Clear pageCount in receipt replacement paths or gate the badge against the current receipt type.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 5e7544a. receiptOptimistic is applied with Onyx.METHOD.MERGE, so omitting pageCount left the replaced receipt's count in place until the server responded. Now sends pageCount: null, matching how localSource is already cleared in the same object, and the type allows null.
Worth noting the server side of this is already handled — Auth's attach/replace update always sends pageCount (null when the new receipt has none), so the correction arrives rather than the field simply being omitted. This closes the optimistic window before it.
The style stopped setting a background when the badge moved to the shared Badge component, but the comment describing that background survived a partial edit and sat above the new one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
replaceReceipt applies its optimistic receipt as a merge, so omitting pageCount left the replaced receipt's count in Onyx. Replacing a multi-page PDF with an image showed the old badge until the server responded. Sending null clears it, matching how localSource is already cleared in the same object. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@codex @MelvinBot review the pr |
|
🤖 Code review — the change is small, well-commented, and the logic (Onyx-driven 1. Implementation doesn't match the PR description (please reconcile)The description promises a "translucent dark pill … reuses
This also undercuts the description's own contrast rationale. The default badge is a solid light-gray pill in light mode, so over a typical white PDF thumbnail it's low-contrast and easy to miss — which is the exact "users don't notice multi-page receipts" problem this PR set out to fix. Either update the description to reflect the simpler default badge, or implement the dark-pill + icon you described. Since design hasn't weighed in yet, worth deciding this now rather than after. 2. "Added unit tests" is checked, but there are no tests in the diffThe author checklist marks "I added unit tests … to prevent regressions", but no test file is included. The gating logic ( Minor
I did not exercise this in a live web session: Otherwise the reasoning comments are genuinely good — the |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ 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". |
Asserts the badge renders for a multi-page PDF and stays hidden for a single page receipt and for one with no count at all, which is also what a PDF uploaded before the backend reported counts looks like. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both main points were right — fixed. 1. Description vs implementation. The description was stale: it still described the translucent-dark-pill-plus-icon I originally built, before design asked for the plain shared 2. Unit tests. Added in ccb27a3, in the existing On the minor points:
|
garrettmknight
left a comment
There was a problem hiding this comment.
Looks good from a product perspective
|
Screenshot looks good but would love to see a mobile screenie too |
|
Nice. Looking good to me. |
|
Thanks!! reviewing it |
how can i run the tests looks like it requires running a backend locally is that correct @ishpaul777 i think we can inject the onyx states using the console let me know if that would sufficient |
|
@abzokhattab yes injecting the onyx states using the console will be sufficient, BE PRs not merged yet, this PR change is backward compatible so its safe to merge first |
|
The Cause: value: {receipt: mergeTransaction.receipt ?? null} // MergeTransaction.ts:347-353
Repro
Screen.Recording.2026-08-13.at.11.29.56.movSuggested fix — gate on the receipt actually being a PDF, which closes this path and any future optimistic writer in one line: const shouldShowReceiptPageCount = receiptPageCount > 1 && Str.isPDF(receiptURIs?.filename ?? '') && !isLoading; |
The merge flow writes the chosen receipt with a merge, so picking an image left the target's old PDF page count in Onyx and the badge rendered "Page 1 of N" over a photo. Gating on the current file type fixes that and any future optimistic writer, since a page count only means anything for a PDF. The merge flow also clears the count explicitly now, so the stale value does not sit in Onyx for other consumers, matching what replaceReceipt already does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Oops, we have a lot of failing pipelines @ishpaul777 cc @MelvinBot |
SignInModal dismisses itself once IS_LOADING_APP settles to false. The back-handling test never sets that flag, but Onyx carries it in from whatever ran earlier in the worker, so the dismiss effect fired in CI and threw on the unmocked dismissModal before any assertion ran.
|
thanks for review @abzokhattab fixed #97813 (comment) and Ci should pass now 🤞 |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb Safari |
abzokhattab
left a comment
There was a problem hiding this comment.
LGTM .. i was only able to test it on web since the backend is not yet deployed
|
🎯 @abzokhattab, thanks for reviewing and testing this PR! 🎉 A payment issue will be created for your review once this PR is deployed to production. If payment is not needed (e.g., regression PR review fix etc), react with 👎 to this comment to prevent the payment issue from being created. |


Explanation of Change
A PDF receipt's thumbnail is a server-generated JPG of page 1. When a multi-page receipt's total sits on page 2 or later, the expense looks like it disagrees with its own receipt, and nothing on screen signals that more pages exist. Hover-zoom does not help — it magnifies the same first page.
This adds a
Page 1 of Nbadge over the receipt in the expense detail view, gated onpageCount > 1.Design settled on the plain shared
Badgewith no icon, so that is what ships: defaultBadgestyling, positioned bottom-left because the receipt action buttons own the top-right corner on hover.It renders as a sibling of the hover-zoom wrapper rather than inside it, so magnifying the receipt does not scale the badge with it, and it waits for the receipt to load because the container stretches while loading and would otherwise strand the badge at the bottom of that taller box.
One thing worth a second look from design: the default
Badgeis a solid light-grey pill in light mode, so over a typical white PDF thumbnail the contrast is modest — and low visibility is the exact problem this PR exists to solve. Happy to switch to a higher-contrast treatment if design prefers.This depends on three backend PRs and shows nothing until they ship, since
pageCountdoes not exist on any receipt today:There is no backfill, so the badge only ever appears on receipts uploaded after the backend ships. The
pageCount > 1gate handles that with no extra code.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/667914
PROPOSAL:
Tests
Automated:
tests/ui/components/MoneyRequestReceiptViewTest.tsxcovers the gating — the badge renders for a multi-page PDF, and stays hidden for a single-page receipt and for one carrying no count at all (which is also what a PDF uploaded before the backend reported counts looks like).Manual steps below require the three backend PRs running locally.
Page 1 of 3badge appears at the bottom-left of the receipt.Offline tests
The page count arrives with the transaction from Onyx and is not fetched separately, so a cached expense keeps its badge offline.
QA Steps
Same as tests. Requires the three backend PRs deployed — the badge cannot appear before then, and only on receipts uploaded after they ship.
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