Conversation
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
…s-first-policy-load
4ff57fe to
82b63c1
Compare
|
@ZhenjaHorbach 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: 82b63c1516
ℹ️ 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 invoiceReceiverPolicyID = report?.invoiceReceiver && 'policyID' in report.invoiceReceiver ? report.invoiceReceiver.policyID : undefined; | ||
| if ((report?.policyID && changedPolicyIDs.has(report.policyID)) || (invoiceReceiverPolicyID && changedPolicyIDs.has(invoiceReceiverPolicyID))) { |
There was a problem hiding this comment.
Include invoice children for receiver-policy loads
This matching only sees report.policyID and report.invoiceReceiver, but B2B invoice report children keep the receiver policy on their parent invoice room (chatReportID/parentReportID), not on the invoice report itself. computeReportName reads reports[chatReportID].invoiceReceiver.policyID for invoice reports, and the existing invoice fixture has the child policyID set to the sender policy while the receiver policy is on the chat report, so a first ReconnectApp load of the receiver policy now recomputes the invoice room but leaves the child invoice report's derived reportName stale in currentValue.
Useful? React with 👍 / 👎.
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp2026-07-13.17.06.38.movAndroid: mWeb Chrome2026-07-13.17.14.31.moviOS: HybridApp2026-07-13.17.06.38.moviOS: mWeb Safari2026-07-13.17.14.31.movMacOS: Chrome / Safari2026-07-13.16.50.00.mov |
|
LGTM! |
|
🚧 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.34-0 🚀
|
|
🤖 I reviewed the changes in this PR against Expensify's help site files under No help site changes are required. This PR is a pure internal performance optimization. It changes how the There is no user-facing change here:
Since the help site documents user-facing product behavior and workflows, and nothing user-facing changed, there's nothing to update in |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.4.34-14 🚀
|
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.4.34-14 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
The
reportAttributesderived value has a "first policy load → recompute all reports" escape hatch. On app refresh,ReconnectAppreturns and merges ~1k policies. BecausepreviousPoliciesstarts empty, the first policy merge hits this branch and forcesneedsFullRecompute = true, which disables incremental updates and re-runs the full per-report attribute pipeline (generateReportAttributes+ red-brick-road reason +computeReportName) for every report in the account.On large accounts this produced an ~11s JS-thread block in a single span, dominated by
computeReportName, and it typically fired as a redundant second full pass right after the cold-start pass.This PR removes the full-recompute special case for first policy load and instead reuses the existing scoped path: only reports that reference a policy whose relevant fields actually changed are recomputed. A brand-new policy already counts as "changed" (
hasPolicyRelevantFieldChanged(undefined, next) returnstrue), so first load is naturally covered — scoping to reports that reference the loaded policies instead of all reports. This drops the cost from O(all reports) to O(reports-with-a-changed-policy).Performance impact: when
ReconnectAppis being called on large account, there is no more ~10s spike in reportAttributes triggered by update to policy collectionFixed Issues
$ #95948
PROPOSAL:
Tests
Test plan based on the previous PR that introduced the change we are getting rid of here: https://github.com/Expensify/App/pull/85025/changes#r2926248175
Offline tests
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