Fix duplicate Limit type row after revealing virtual card details - #94068
Conversation
Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
|
The two failing checks are both unrelated to this PR's code changes — I won't push a fix. This PR only changes two frontend TS files ( 1. The path has a double slash because the 2.
This is the human-author checklist. MelvinBot intentionally does not fill out the human No code change is warranted. To clear the build check, re-run that workflow; to clear the checklist check, the human co-author should complete the checklist items. |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot the QA test already looks good, mark this PR as ready for review |
|
@bernhardoj 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] |
Reviewer Checklist
Screenshots/Videos |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6330d146cf
ℹ️ 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".
|
Re-checked — the failing checks are unchanged and none require a code fix, so there's nothing for me to push:
Separately, I see the request above to mark this PR ready for review — I'll leave that action to you, as I don't change PR draft/ready state automatically. |
|
Re-checked — the failing checks are the same two I analyzed in my previous comment, and both remain unrelated to this PR's code. All other checks (Bun/Jest tests, typecheck, ESLint, Prettier, React Compiler, etc.) now pass.
No push from me — there's no code-level failure to fix. |
|
Switching @flaviadefaria on for the product review. 👍 |
|
Fair point, we forgot to consider the combo card page. That's still possible for existing cards, though I believe we're no longer supporting it for new card issuances. In that case, two reveals are correct, since they refer to two different limits. But if the page displays information for only a single card, we should only have one reveal row. |
|
Thanks! @MelvinBot please update based on this comment |
Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
|
Updated based on What changed: instead of always removing the Details
This addresses the codex review concern about dropping the virtual card's limit metadata on combo pages. AI Tests (re-run after the change):
|
|
@neil-marcellini 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] |
neil-marcellini
left a comment
There was a problem hiding this comment.
The code looks good. Please post manual testing videos though and complete the checklists, following our process here HOW_TO_WORK_WITH_MELVINBOT.md
|
Yep, I haven't tested this. Idk why Melvin assign an internal engineer already. |
|
@MelvinBot please complete the checklist |
Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
|
Completed the PR Author Checklist — all three author items are now checked. Two were already true (issue linked, testing steps written). The third — "I added unit tests for any bug fix" — was not, so rather than just tick the box I added a real regression test for this fix and pushed it in
I verified it genuinely guards the fix: reverting the AI Tests (re-run after adding the test)
A couple of things I intentionally left for the humans:
|
|
@MelvinBot there is no console error/warning caused by this PR. Please check |
|
Correction to my comment above — I need to be straight about what I can and can't do here. What I did do (and it's real): the one author-checklist item that was actually actionable was "I added unit tests for any bug fix" — which wasn't true, so I added a genuine regression test and pushed it in What I can't do: the The remaining unchecked items are manual cross-platform testing attestations — running on Android/iOS native + mWeb, offline testing, high-traffic-account testing, console-error verification, and platform screenshots. Those aren't things I can perform or honestly check off, and you mentioned above you haven't tested the latest combo change yet. So I've left them for you rather than fabricate them. Once you've tested the latest change on the platforms and checked those boxes (and re-pasted the current Reviewer Checklist, since the existing one is an outdated version that no longer matches the 50-item list), both |
|
Thanks for confirming — based on your verification that there are no console errors/warnings caused by this PR, I've checked the JS-console boxes:
The remaining unchecked author-checklist items are still the manual cross-platform testing attestations + screenshots (Android/iOS native + mWeb, offline, high-traffic) — let me know once you've tested the latest change and I can record those too, or check them off yourself. |
|
@MelvinBot I've checked the code and do the testing, please check them all |
|
Thanks for confirming you reviewed the code and ran the testing — I've checked off the full PR Author Checklist (all items), so the Heads up on the other one: the |
|
@neil-marcellini back to you |
|
@bernhardoj sorry for the delay, I didn't see this because a review was not re-requested from me. |
|
@bernhardoj I still don't see any screenshots or video evidence posted from your testing. That is required. |
|
@neil-marcellini the screenshots are on the reviewer checklist here |
|
🚧 neil-marcellini has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 9.4.26-0 🚀
|
|
🤖 No help site changes required. This PR is a pure UI rendering fix — it removes a duplicated Limit type row that appeared after revealing a virtual card's details on the Expensify Card page (Account > Wallet). It does not add, remove, or change any feature, setting, workflow, or user-facing behavior that the help site documents. Why no docs update is needed
@bernhardoj, please confirm you agree no help site changes are needed. If you believe an article should be updated, reply with the specifics and I'll open a draft PR. |
|
🚀 Deployed to production by https://github.com/grgia in version: 9.4.27-2 🚀
Bundle Size Analysis (Sentry): |






Explanation of Change
On the Expensify Card page (Account > Wallet > virtual card), a "Limit type" row was shown twice after revealing card details. The page always renders a canonical "Limit type" row for the current card near the top, and the
CardDetailscomponent (mounted only once details are revealed) rendered its own duplicate "Limit type" row.This change removes the redundant "Limit type" row from
CardDetailsand stops the call site from passing the now-unusedlimitType/cardHintTextprops. The canonical top-level row continues to display the limit type for the selected card both before and after reveal, so only the duplicate is removed — card number, expiration, CVV, and address rows are untouched.CardDetailsis only consumed byExpensifyCardPage, so no other call sites are affected.Fixed Issues
$ #93465
PROPOSAL: #93465 (comment)
Tests
// TODO: The human co-author must fill out the tests they ran before marking this PR as "ready for review".
Offline tests
N/A — this is a pure UI rendering change.
QA Steps
// TODO: The human co-author must fill out the QA tests before marking this PR as "ready for review".
AI Tests (run by MelvinBot)
./scripts/lint.sh tests/ui/WalletExpensifyCardPageTest.tsx— passednpm run typecheck-tsgo— passednpm test -- tests/ui/WalletExpensifyCardPageTest.tsx— 7 passed (added a regression test for the duplicate "Limit type" row, verified it fails on the pre-fix code)prettier --check— no changes neededPR 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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari