Conversation
…ach Concierge Co-authored-by: Chavda Sachin <ChavdaSachin@users.noreply.github.com>
Co-authored-by: Chavda Sachin <ChavdaSachin@users.noreply.github.com>
|
Fixed and pushed — What was failing: Fix: ran The other red check, VerificationRun locally after the fix:
Could not run: CI after the push confirms |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
🤖 Web test steps verification results: The "Copy in progress" modal now shows a working X (Close) icon that dismisses the modal. The failure / Concierge-notification path (VERIFY 4-6) could not be exercised, because deleting the target workspace mid-copy requires a second browser tab, which is out of scope for this single-session web test.
Setup note: the handed-off session was mid-onboarding, so I completed the onboarding wizard first, renamed the resulting default workspace to "Workspace A", and created "Workspace B". No console errors were observed. To close out VERIFY 4-6 you need either a second tab/session to delete the target workspace mid-copy, or a way to force the copy-settings command to fail from a single tab. |














Explanation of Change
The Concierge failure notification for a bulk "Copy settings" run is requested from exactly one place: the in-progress step's
onCancel, which callsrequestCopyPolicySettingsNotification(true)(shouldOnlyNotifyOnFailure). That step rendered withshouldShowCancelButton: false, an emptycancelText, and noshouldShowDismissIcon, so it had no X and no Cancel button.onCancelwas reachable only by backdrop press, Escape, or browser back — a user who closes the modal the normal way never got there, so the failure-only notification was never requested and no Concierge message arrived.The one visible button, "Let me know when it's done", calls
requestCopyPolicySettingsNotification()with the defaultfalse. That is the notify-either-way path, not the failure-only path, and it swaps the modal to the "we'll ping you in Concierge" step instead of closing it.This PR passes
shouldShowDismissIcon: trueon the in-progress step (and threads the prop through toConfirmModal, which already wires the X toonCancel). Closing the progress modal while the copy is still running now requests the failure-only Concierge notification, which is what the QA step expects.Behavior on the other steps is unchanged. The failure step still dismisses without requesting a notification, because the error is already on screen.
AI Tests
Run locally against the changed files, per the App test matrix:
npm test -- tests/unit/CopyPolicySettingsProgressModalTest.tsx— passed (28 tests, including 3 new/tightened assertions)npm run lint-changed— passednpm run typecheck— passednpm run spell-changed— passed (2 files, 0 issues)npm run react-compiler-compliance-check check <changed component>— passedCould not run: browser verification of the new X. The automated web session handed to this run was already broken before it started (onboarding replay divergence, app rendered a blank page), so the in-progress modal was never reached. The screenshots in the sections below still need to be captured by a human.
Fixed Issues
$ #100864
PROPOSAL: #100864 (comment)
Tests
// TODO: The human co-author must fill out the tests they ran before marking this PR as "ready for review".
// Suggested starting point, based on the repro in the linked issue:
// 1. Create Workspace A and Workspace B on the same account.
// 2. Open Workspace A > Copy settings, pick Workspace B as the target, select Profile / Overview, and continue to the confirm page.
// 3. Click Copy, then verify the "Copy in progress" modal now shows an X in the top-right corner.
// 4. In a second tab (OldDot), delete Workspace B so the copy fails.
// 5. Click the X to close the progress modal before the job finishes, then open Concierge and verify a message about the failed copy arrives.
// 6. Repeat steps 1-4 but leave the modal open, and verify the failure modal still shows Try again / Dismiss, and that dismissing it does NOT post a Concierge message.
Offline tests
// TODO: The human co-author must fill this out. Note that the notification request is a normal queued write with no conflict resolver, so it is expected to be sent once connectivity returns.
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
// TODO: The human co-author must fill out the QA steps before marking this PR as "ready for review".
// Note for whoever writes these: steps 12 and 13 of the original repro cannot both be satisfied in a single run. Step 12 needs the modal open when the job fails; step 13 needs it closed before the job finishes. They should be split into two separate QA runs.
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