Conversation
…nsifyCardRule After SetExpensifyCardRule, the backend returns onyxMethod: set which replaces the entire expensifyCardSettings object, wiping hasOnceLoaded. The useEffect guard then fails and triggers an unnecessary API call. Add a useRef to track whether openPolicyExpensifyCardsPage was already called during this mount, making the guard immune to Onyx state resets. Co-authored-by: Carlos Martins <luacmartins@users.noreply.github.com>
|
The All three retry attempts failed with the same TLS error. Please re-run the failed job to get a clean run. |
|
The The typecheck itself never ran — the job couldn't even clone the repo. Please re-run the failed workflow to resolve this. |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
melvin-bot
Bot
requested review from
aimane-chnaif
and removed request for
a team
|
@aimane-chnaif 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] |
melvin-bot
Bot
requested review from
trjExpensify
and removed request for
a team
|
@codex review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // We use a ref to track this because SetExpensifyCardRule returns onyxMethod: set, which replaces the entire | ||
| // expensifyCardSettings object and wipes hasOnceLoaded, causing an unnecessary reload. | ||
| if (hasLoadedCardSettingsRef.current || !expensifyCardSettings || expensifyCardSettings?.isLoading || expensifyCardSettings?.hasOnceLoaded) { | ||
| return; |
There was a problem hiding this comment.
❌ PERF-16 (docs)
The ref-based guard does not fully prevent double execution. The ref hasLoadedCardSettingsRef is only set to true when openPolicyExpensifyCardsPage is actually called (line 143). In the most common scenario described in the PR -- where the parent already loaded the data (hasOnceLoaded is true) -- the effect returns early and the ref stays false. When the backend SET response later wipes hasOnceLoaded, the ref is still false, so the guard falls through and the unnecessary API call fires anyway. The fix only prevents a second unnecessary reload, not the first one.
Restructure the effect so the ref is set to true whenever data is already loaded, not just when the API call is made:
useEffect(() => {
if (hasLoadedCardSettingsRef.current || !expensifyCardSettings || expensifyCardSettings?.isLoading) {
return;
}
hasLoadedCardSettingsRef.current = true;
if (expensifyCardSettings?.hasOnceLoaded) {
return;
}
openPolicyExpensifyCardsPage(policyID, defaultFundID);
}, [defaultFundID, expensifyCardSettings, policyID]);This way, once the effect observes that data has been loaded (either by the parent or by this effect), the ref prevents any future invocations from triggering the API call even if hasOnceLoaded gets wiped by a backend SET response.
Reviewed at: 0ba2f9e | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
Is this regression/follow-up of previous PR? |
|
@aimane-chnaif yes |
![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: 0ba2f9ef64
ℹ️ 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".
| hasLoadedCardSettingsRef.current = true; | ||
| openPolicyExpensifyCardsPage(policyID, defaultFundID); |
There was a problem hiding this comment.
Setting hasLoadedCardSettingsRef.current = true before dispatching openPolicyExpensifyCardsPage makes loading a one-shot attempt for the entire mount. If OPEN_POLICY_EXPENSIFY_CARDS_PAGE fails (its failureData in src/libs/actions/Policy/Policy.ts clears isLoading but does not mark hasOnceLoaded), this guard still blocks every later retry even though settings never loaded. In transient online failure scenarios, the page can remain stuck with no cards until the user leaves and re-enters.
Useful? React with 👍 / 👎.
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
Ecard spend rules follow up 👍
|
I'll take a look at this one soon |
JS00001
left a comment
JS00001
left a comment
There was a problem hiding this comment.
Is a ref the correct solution here? Can we store a var in the finallyData that we can access via the onyx data?
If we use a ref, wouldnt the refs value get cleared when navigating away & navigating backl?
| if (!expensifyCardSettings || expensifyCardSettings?.isLoading || expensifyCardSettings?.hasOnceLoaded) { | ||
| // We use a ref to track this because SetExpensifyCardRule returns onyxMethod: set, which replaces the entire | ||
| // expensifyCardSettings object and wipes hasOnceLoaded, causing an unnecessary reload. | ||
| if (hasLoadedCardSettingsRef.current || !expensifyCardSettings || expensifyCardSettings?.isLoading || expensifyCardSettings?.hasOnceLoaded) { |
There was a problem hiding this comment.
Current fix can not resolve an issue
CleanShot.2026-04-23.at.01.12.41.1.mp4
If we decide to fix this on the client I think we can simplify the condition to only open openPolicyExpensifyCardsPage when expensifyCardSettings is undefined.
if (!expensifyCardSettings) {
openPolicyExpensifyCardsPage(policyID, defaultFundID);
}There was a problem hiding this comment.
@luacmartins I just referenced the initial change, and it looks like there’s a mismatch in the return condition when expensifyCardSettings is undefined and the suggested update only prevent infinity request 😄
|
The only failing check is |
|
Implemented the proposed fix. Added |
|
@MelvinBot missed updating hasOnceLoaded: true in the CleanShot.2026-04-28.at.18.00.06.1.mp4 |
|
Good catch — The fix is to add I can only make code changes when directed by the PR author, an assignee, or an existing co-author. To have me implement this, the PR assignee or author should reply with a request tagging me. |
|
@MelvinBot implement this one |
|
The fix for However, I can only make code changes when directed by the PR author, an assignee, or an existing co-author. |
cc: @luacmartins |
|
@MelvinBot implement #88365 (comment) |
The deleteExpensifyCardRule function was missing hasOnceLoaded: true in its successData and failureData blocks. When the backend returns an onyxMethod: set response after deleting a rule, it wipes hasOnceLoaded, triggering an unnecessary OpenPolicyExpensifyCardsPage reload — the same bug that setExpensifyCardRule already guards against. Co-authored-by: luacmartins <luacmartins@users.noreply.github.com> Co-authored-by: Carlos Martins <luacmartins@users.noreply.github.com>
|
Added |
suneox
left a comment
suneox
left a comment
There was a problem hiding this comment.
The remaining issue with reloading Expensify Cards after deleting a rule has been fixed.
CleanShot.2026-04-28.at.18.11.13.1.mp4
melvin-bot
Bot
changed the title
|
🎯 @suneox, 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. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @cead22 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/cead22 in version: 9.3.65-0 🚀
Bundle Size Analysis (Sentry): |
|
I reviewed the changes in this PR. It's a purely internal bug fix that prevents an unnecessary No help site documentation changes are required — there are no user-facing changes (no new features, UI changes, renamed settings, or altered workflows). The existing help site articles that reference card rules remain accurate. |
|
Hello @JS00001 @luacmartins |
|
@lanitochka17 sorry, this should be for card rules, not merchant rules |
|
@JS00001 We can see only Expenses and Merchant rules. Did we miss anything? Recording.3736.mp4 |
|
You have to have expensify cards enabled on the policy, let me update the test steps |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.3.65-6 🚀
|
|
🤖 Payment issue created: #89505 |
10 participants
Explanation of Change
After
SetExpensifyCardRule, the backend returnsonyxMethod: setwhich replaces the entireexpensifyCardSettingsobject, wipinghasOnceLoaded. This causes theuseEffectguard inSpendRuleCardPageto fail, triggering an unnecessaryopenPolicyExpensifyCardsPageAPI call every time a spend rule is added or edited.This PR adds a
useRefto track whetheropenPolicyExpensifyCardsPagewas already called during the current mount, making the guard immune to Onyx state resets from backend SET responses.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/624064
Tests
Workspaces > RulesChoose Cardsoption to open SpendRuleCardPageOpenPolicyExpensifyCardsPageAPI call and no loading status is displayedOpenPolicyExpensifyCardsPageafter creating/changing a rule (step 3)Offline tests
N/A
QA Steps
N/A
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
Screen.Recording.2026-04-21.at.12.47.48.PM.mov
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari