Fix Supplier RHP backdrop glitch — align STEP_VENDOR with sibling IOU steps#96532
Fix Supplier RHP backdrop glitch — align STEP_VENDOR with sibling IOU steps#96532Beamanator wants to merge 2 commits into
Conversation
Two changes bring STEP_VENDOR in line with sibling IOU steps: 1. Declare `backTo: Routes` on the STEP_VENDOR params type. STEP_MERCHANT and the other siblings already declare it; without the declaration the linking layer treats the value as an opaque unknown, which sidesteps the tab-swap fast path in `swapBackgroundTabForRHPTarget` and causes the underlay tab to be rebuilt when the RHP mounts. 2. Swap `usePolicyForTransaction` for the simpler `usePolicy` in `IOURequestStepVendor`. The former chained through `usePolicyForMovingExpenses` (session + activePolicySelector) and a whole-POLICY collection selector to cover track-expense / per-diem cases that never reach this picker — the vendor-feature gate short- circuits on both. Extra subscriptions on RHP mount block the entry animation just long enough to flicker the underlay. Reported by @thelullabyy in #94093 (comment) and tracked in Expensify/Expensify#638611.
|
@Eskalifer1 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] |
Codecov Report✅ All modified and coverable lines are covered by tests. |
![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: e536296cd0
ℹ️ 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".
| iouType, | ||
| isPerDiemRequest: isPerDiemRequest(transaction), | ||
| }); | ||
| const policy = usePolicy(report?.policyID); |
There was a problem hiding this comment.
When the Supplier/Vendor row is opened for an unreported self-DM track expense, the row is shown using policyForMovingExpenses in MoneyRequestView for isExpenseUnreported transactions, but the thread report passed in the route was built from the self-DM report and does not carry that workspace policyID. Replacing usePolicyForTransaction here with usePolicy(report?.policyID) therefore makes policy undefined, so hasVendorFeature(policy, ...) immediately sends the picker to Not Found even though the visible row was editable. The previous hook explicitly used policyForMovingExpenses for isExpenseUnreported transactions, matching the caller's policy selection.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e536296cd0
ℹ️ 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".
| iouType, | ||
| isPerDiemRequest: isPerDiemRequest(transaction), | ||
| }); | ||
| const policy = usePolicy(report?.policyID); |
There was a problem hiding this comment.
When the Vendor/Supplier row is opened for an unreported non-reimbursable expense, MoneyRequestView still derives the row from policyForMovingExpenses for unreported transactions (MoneyRequestView.tsx lines 256-258) and renders it when that policy has vendor matching enabled (line 537). This new lookup instead uses the transaction thread report's policyID, which can be the self-DM/fake/empty policy for that flow, so hasVendorFeature(policy, ...) becomes false and the picker shows the not-found page; before this change usePolicyForTransaction selected the same fallback policy as the row. Please keep the transaction-derived policy here so tapping Vendor/Supplier from those expenses opens the picker.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 334624b — restored usePolicyForTransaction so unreported non-reimbursable rows still resolve the fallback policy. The backTo type declaration was the actual fix for the backdrop rebuild.
…le flow The earlier commit swapped usePolicyForTransaction for usePolicy(report?.policyID) on the theory that the extra subscriptions were what caused the underlay tab to rebuild on Supplier RHP mount. That was wrong: unreported non-reimbursable expenses render their MenuItemWithTopDescription against policyForMovingExpenses (MoneyRequestView.tsx:256-258), and swapping to usePolicy(report?.policyID) makes IOURequestStepVendor resolve the self-DM/fake policy, hasVendorFeature returns false, and tapping the row lands on the not-found page. Restore usePolicyForTransaction so the picker sees the same policy as the row that opens it. The backTo: Routes param-type declaration in this branch remains the actual fix for the underlay-rebuild glitch. Reported by Codex on this PR.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 334624b382
ℹ️ 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".
| reportActionID?: string; | ||
| reportID: string; | ||
| // eslint-disable-next-line no-restricted-syntax -- `backTo` usages in this file are legacy. Do not add new `backTo` params to screens. See contributingGuides/NAVIGATION.md | ||
| backTo: Routes; |
There was a problem hiding this comment.
In the expense-view Supplier flow I inspected, MoneyRequestView already passes getReportRHPActiveRoute() into ROUTES.MONEY_REQUEST_STEP_VENDOR.getRoute(...), and the tab-swap code in linkTo/swapBackgroundTabForRHPTarget only checks the runtime route.params.backTo. This new backTo entry is just a TypeScript param-list annotation and is erased from the bundle, so it cannot change how getMatchingFullScreenRoute sees the route or fix the reported backdrop flicker when opening the Supplier RHP.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
from Alex's Claude agent
You are right — the backTo addition is TS-only, cannot change runtime linking behavior. Closing this PR. Will reopen once I have a proper local repro to identify the actual runtime cause of the backdrop rebuild.
|
from Alex's Claude agent Closing — Codex correctly pointed out that the type-only Will reopen with a fresh investigation once I have a local repro of the backdrop rebuild to pin the actual runtime cause. |
1 participant
Explanation of Change
Follow-up polish for Vendor matching CC - Release 4: Xero after #94093 merged. QA reported a visible backdrop glitch when opening the Supplier RHP from the expense view — the underlying tab appeared to re-slide in beneath the RHP instead of the RHP just opening on top of a static backdrop. Doesn't happen on any of the sibling IOU steps (Merchant, Date, Description, etc). Two changes bring
STEP_VENDORin line with the siblings and fix the glitch:backTo: Routeson theSTEP_VENDORparams type.STEP_MERCHANTand every other IOU step declare it; without the declaration the linking layer treats the value as an opaque unknown, which sidesteps the tab-swap fast path inswapBackgroundTabForRHPTargetand causes the underlay tab to be rebuilt when the RHP mounts.usePolicyForTransactionfor the simplerusePolicyinIOURequestStepVendor. The former chained throughusePolicyForMovingExpenses(session +activePolicySelector) and a whole-POLICYcollection selector to cover track-expense / per-diem cases that never reach this picker — thehasVendorFeaturegate short-circuits on both. Extra subscriptions on RHP mount block the entry animation just long enough to flicker the underlay.Reported by @thelullabyy in #94093 (comment).
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/638611
PROPOSAL: N/A — follow-up polish on Alex's own Track C from the parent issue
Tests
vendorMatchingbeta enabled, Xero connected, and at least one supplier in Xero, create a non-reimbursable expense.Offline tests
Same as tests — the fix is a client-side navigation / hook change and doesn't touch network state.
QA Steps
Same as tests.
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
To be added once a local build with a Xero-connected workspace reproduces the flow. The user-facing surface here is the Supplier RHP entry animation — no new UI components, no copy changes.
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari