Skip report name recompute for badge-only policy changes in reportAttributes - #97093
Conversation
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d816bbbee3
ℹ️ 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 was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f51a475399
ℹ️ 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".
ReviewThe optimization is well-scoped and the approach is sound: only the 🟠
|
Regression Analysis:1 —
2 —
Fix directionExtend the
Worth fixing these edge cases, similar to |
…s-skip-name-recompute
Fixed, though deliberately not with the suggested Instead, the recompute is scoped to the report shapes that actually read each field: |
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: ac950abc00
ℹ️ 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".
| const emptyNameChanged = isPolicyFieldListEmpty(prevPolicy ?? undefined) !== isPolicyFieldListEmpty(nextPolicy ?? undefined); | ||
| if (!hasPolicyRelevantFieldChanged(prevPolicy, nextPolicy) && !nameChanged && !emptyNameChanged) { |
There was a problem hiding this comment.
Track non-empty fieldList changes before skipping policies
When policies are seeded from disk and the server later returns a policy whose fieldList changed from one non-empty set to another, this emptiness-only check makes the POLICY trigger look irrelevant and the incremental path returns currentValue. However report attributes call hasVisibleReportFieldViolations(...), which reads the actual policy.fieldList definitions to decide the RBR/Fix badge, so adding/removing/disabling a report field can leave existing expense or invoice reports with stale badges until some unrelated report update happens.
Useful? React with 👍 / 👎.
|
🤖 Regression review — I traced the new "reuse cached name for badge-only policy changes" logic against every policy field the name pipeline reads. Found one real (but narrow) regression; the rest of the mechanics check out. Regression: a
|
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required on this one.
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 mountiny 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/mountiny in version: 9.4.46-0 🚀
|
|
🤖 Help site review: no docs changes required. I reviewed the changes in this PR against the help site articles under This is a pure internal performance optimization with no user-facing behavior change. It only affects how the Since there's no behavior change to describe, there's nothing for the help site to document, so I did not create a draft docs PR. @TMisiukiewicz, please confirm you agree this change has no user-facing impact that the help site would need to document. If you think a help article is affected, let me know which article/behavior and I'll draft the update. |
Explanation of Change
Opening the Spend tab for the first time fires
OpenSearchPage, whose server response merges the fullpolicy_collection (5.6k policies on large accounts).getCollectionDeltais a reference-equality scan, so the merge surfaces ~all policies as "changed" andreportAttributesrecomputed the name (computeReportName) for ~all 8245 reports — a ~30s main-thread hang.Root cause:
computeReportNamederives a report's name frompolicy.name/achAccountonly (viagetPolicyName), never from the badge-relevant fieldshasPolicyRelevantFieldChangedtracks (type/approvalMode/reimbursementChoice/autoReimbursementLimit/role/autoReimbursement.limit). Those fields drive LHN badges, not names. Recomputing names for badge-only policy changes was wasted work.Fix:
achAccount) actually changed. A report pulled into the pass solely by a policy badge-field change reuses its cached name and skipscomputeReportName. This is consistent with existing behavior where a policy-name-only change was already not recomputed.previousPoliciesvalue-baseline on the startup flush so the first policy trigger has a real baseline to diff against.Tests: added/updated unit tests in
tests/unit/reportAttributesTest.ts(25 total pass), covering: badge-only change keeps cached name; policy name change forces recompute; invoice receiver-policy role change still recomputes the invoice name; and a coalesced badge-change + transaction-update flush still recomputes the name.Performance improvement:
4x CPU throttling, opening Spend tab goes from ~30s to ~5s
Before vs After
before.mov
after.mov
Fixed Issues
$ #97095
PROPOSAL:
Tests
Offline tests
N/A
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
after.mov