Conversation
Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
…ly policy Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
…cySnapshotFallback Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
🤖 Fixed by merging Failures analyzed:
This PR only changed Fix applied: |
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
🤖 Correction to my earlier comment — the two failing test jobs are a pre-existing, The two failures, on the latest run (29273186930):
Why this isn't this PR's fault:
Recommendation: These need a separate fix on The |
| const policy = selectedPolicyIDs.length === 1 ? policies?.[`${ONYXKEYS.COLLECTION.POLICY}${selectedPolicyIDs.at(0)}`] : undefined; | ||
| // Read the policy through the Search-snapshot fallback: this hook renders outside SearchScopeProvider, so on a | ||
| // fresh load / cache clear the policy may only exist in the search snapshot and not yet in live Onyx. | ||
| const policy = selectedPolicyIDs.length === 1 ? getPolicyFromSearchSnapshot(selectedPolicyIDs.at(0), currentSearchResults?.data, policies) : undefined; |
There was a problem hiding this comment.
@MelvinBot Can we use currentSearchResults?.data?. directly instead of getPolicyFromSearchSnapshot since getPolicyFromSearchSnapshot prefer policy from live onyx
App/src/libs/actions/Search.ts
Lines 139 to 147 in 1e4e79e
There was a problem hiding this comment.
We could, but there's a real behavioral difference — both approaches fix the reported bug, they only differ in the tie-break when the policy exists in both live Onyx and the snapshot.
getPolicyFromSearchSnapshotis live-first:policies?.[policy_<id>] ?? snapshotPolicy.- Reading
currentSearchResults?.data?.[policy_]directly is snapshot-only.
Both resolve the fresh-load case (policy missing from live Onyx → fall back to / read from snapshot), so the reported options-disappearing bug is fixed either way. The trade-off:
- Live-first (current PR): if the user just changed their workspace's accounting connection,
getConnectedIntegrationreflects that immediately. It's also consistent with the report read right below it, which routes through the siblinggetReportFromSearchSnapshothelper, and reuses the shared helper instead of duplicating the key construction inline. - Snapshot-only (your suggestion): more robust only if live Onyx can hold a partial policy that's missing
connections— in that case live-first would return the incomplete live policy and re-hide the options. If that scenario can't happen on this path, the two are equivalent aside from staleness.
My recommendation is to keep the helper: it gives current workspace settings when available, keeps the two reads consistent, and avoids inlining the collection-key string. The only reason to switch to snapshot-direct is if we expect the live policy to be present-but-incomplete here.
If you'd prefer snapshot-direct, I'm happy to make the change — just confirm and I'll push it (and update the faithful test mocks accordingly).
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
@MelvinBot The issue is on the first load live policy come without policy.connections, so we need to use the snapshotPolicy instead
There was a problem hiding this comment.
Done — pushed 972d7fb. Both policy reads in the export block now read from the Search snapshot instead of the live-first helper:
useSearchBulkActions.ts:1473(single-policypolicy)useSearchBulkActions.ts:1492(reportPolicy)
One deliberate deviation: I read currentSearchResults?.data?.[policy_<id>] ?? policies?.[policy_<id>] — snapshot-first with a live fallback — rather than snapshot-only. A pure snapshot-only read regresses the existing test useSearchBulkActionsExportTest.ts:377: there the report arrives fresh from search (snapshot only) while the policy is already loaded in live Onyx and never written to the snapshot, so a snapshot-only read returns undefined and re-hides the integration options. Snapshot-first fixes your concern (the snapshot now wins over live) while keeping that case working, and it mirrors the sibling getReportFromSearchSnapshot which is already snapshot ?? live.
Types are unchanged: this is the same expression getPolicyFromSearchSnapshot uses internally (just snapshot-first), and it matches the existing direct snapshot read at useSearchBulkActions.ts:504. I couldn't run the local typecheck/lint/test tooling in this environment, so I verified statically against those precedents — CI will run the full checks.
If you truly want snapshot-only (dropping the live fallback), let me know and I'll adjust that test to match, but flagging it because it would drop the policy-already-in-live-Onyx case.
There was a problem hiding this comment.
Exactly right — that's the case I was unsure about, and you've confirmed it: on first load the live policy can be present but without its connections yet, so a live-first read (getPolicyFromSearchSnapshot) would return the connection-less live policy and drop the whole integration block.
The snapshot-first read is already in 972d7fb (currentSearchResults?.data?.[key] ?? policies?.[key]), so the snapshot policy — which does carry connections — now wins.
To lock that behavior in, I just pushed 11bd281 adding a regression test for this precise scenario: a live policy of {id} with no connections plus a fully-connected snapshot policy. It fails under a live-first read (options disappear) and passes with the snapshot-first read. Full suite:
✓ offers the integration export options when the report is only in the search snapshot (fresh load)
✓ offers the integration export options when the policy is only in the search snapshot (fresh load)
✓ prefers the snapshot policy when the live policy is present but has not loaded its connections (fresh load)
✓ does NOT offer the integration export options when the report is absent from both snapshot and live Onyx
Tests: 4 passed, 4 total
|
@MelvinBot Please update TestsPreconditions
Offline testsN/A. QA StepsSame as Tests step. PR Author ChecklistPlease add the full checklist and mark all items as checked |
|
Updated the PR description ✅
One heads-up: the Screenshots/Videos section is still empty (the platform |
…e fallback)
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
…nnections
Guards the snapshot-first export policy read against a live-first regression:
on a fresh load the live policy can be present but without its connections yet,
so a live-first read would drop the integration options. This test fails under
a live-first read and passes with the snapshot-first read.
Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
|
@MelvinBot Please merge main |
…cySnapshotFallback Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
🤖 Merged |
|
I'll pause work on this for now because I'm unable to reproduce the original bug. |
@MelvinBot please close this PR |
|
Closing this PR as requested — you weren't able to reproduce the original bug, so we're pausing work here. Feel free to reopen or ping me if the issue resurfaces and you'd like to pick it back up. |
Explanation of Change
On the Search Reports tab, the bulk-action Export dropdown was missing the Export to
<integration>and Mark as exported options on a fresh load / after a cache clear.SearchBulkActionsButtonrenders outsideSearchScopeProvider, souseOnyxreads are not auto-redirected to the Search snapshot. On a fresh load the selected policy may only exist in the Search snapshot and not yet in live Onyx. PR #93297 already fixed the equivalent problem for the report lookup by falling back togetReportFromSearchSnapshot(...), but deliberately left the two policy reads ingetExportOptions()on live Onyx. When the policy is absent from live Onyx,getConnectedIntegration(policy)returnsundefined, the wholeif (connectedIntegration)block is skipped, and both integration options disappear.This PR mirrors that snapshot fallback for the policy reads in
src/hooks/useSearchBulkActions.ts:policyread now usesgetPolicyFromSearchSnapshot(...).reportPolicyread insidecanReportBeExportednow usesgetPolicyFromSearchSnapshot(...).getPolicyFromSearchSnapshotprefers live Onyx and falls back to the snapshot, so current workspace settings still win when available — the only behavioral change is that the integration gate no longer requires the policy to already be present in live Onyx.Tests: the existing
useSearchBulkActionsExportTest.tsmockedgetPolicyFromSearchSnapshotas a no-op; it is now a faithful mock (live-first, snapshot-fallback), and the same faithful mock was added to the sibling suites that mock@libs/actions/Search. A new test covers the fresh-load case where the policy exists only in the search snapshot and verifies both integration options appear.Fixed Issues
$ #95820
PROPOSAL:
Tests
Preconditions
Approvedstatus.Offline tests
N/A.
QA Steps
Same as Tests step.
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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari