extract DeleteWorkspaceFlow component from WorkspacesListPage#93717
extract DeleteWorkspaceFlow component from WorkspacesListPage#93717WojtekBoman wants to merge 1 commit into
Conversation
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
WojtekBoman
force-pushed
the
perf/workspaces-list-page-composition-1
branch
from
f73cec4 to
6c06845
Compare
|
@abzokhattab 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] |
|
@abzokhattab All yours! |
|
Great ... reviewing! |
|
@MelvinBot, review the PR |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return; | ||
| } | ||
| isErrorModalShowingRef.current = true; | ||
| showConfirmModal({ |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The error-modal block in this effect is duplicated almost verbatim. The showConfirmModal({...}).then(...) call at the bottom of the effect (the non-offline branch) is identical to the one in the offline branch above it (same title, prompt: policyLatestErrorMessage, confirmText, cancelText, success: false, shouldShowCancelButton: false, and the same .then handler calling hideDeleteWorkspaceErrorModal()). Maintaining two copies risks them drifting apart.
Extract the shared modal into a single local helper and call it from both branches:
const showErrorModal = () => {
if (isErrorModalShowingRef.current) {
return;
}
isErrorModalShowingRef.current = true;
showConfirmModal({
title: translate('workspace.common.delete'),
prompt: policyLatestErrorMessage,
confirmText: translate('common.buttonConfirm'),
cancelText: translate('common.cancel'),
success: false,
shouldShowCancelButton: false,
}).then(() => {
isErrorModalShowingRef.current = false;
hideDeleteWorkspaceErrorModal();
});
};Then call showErrorModal() in both the offline branch and the post-pending-delete branch.
Reviewed at: 6c06845 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
![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: 6c06845b55
ℹ️ 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 {reportsToArchive, transactionViolations} = useTransactionViolationOfWorkspace(policyID); | ||
| const [accountIDToLogin] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST, {selector: accountIDToLoginSelector(reportsToArchive)}); | ||
|
|
||
| const isLoadingData = isLoadingOnyxValue(policiesResult, accountResult, amountOwedResult, privateSubscriptionResult, cardFeedsResult, cardsListResult); |
There was a problem hiding this comment.
When the delete flow is mounted on demand, useTransactionViolationOfWorkspace() starts loading the report and transaction collections at the same time as this readiness check, but isLoadingData does not include those Onyx results. On accounts where reports/transactions hydrate slower than the policy/account/card keys, the effect can call continueDeleteWorkspace() with an empty reportsToArchive / transactionViolations, so deleteWorkspace skips archiving the workspace chats and clearing their violations. Please include those loading states (or otherwise defer until the hook's data is ready) before starting the deletion.
Useful? React with 👍 / 👎.
melvin-bot
Bot
requested review from
joekaufmanexpensify
and removed request for
a team
Code reviewReviewed this as a behavior-preserving refactor. The extraction is clean and the ported logic is faithful — I verified the inlined helpers against their originals:
No blockers found. A few things worth confirming before merge: 1.
|
3 participants
Explanation of Change
This is the first in a series of smaller PRs splitting a larger
WorkspacesListPagecomposition refactor (#93293) into incremental steps to catch regressions early.This PR extracts all workspace deletion logic from
WorkspacesListPageinto a newDeleteWorkspaceFlowcomponent that mounts only when a deletion is in progress. Heavy Onyx subscriptions (full policy collection, card feeds, reports, transaction violations) are now scoped to the flow's lifetime instead of living on the list page for every row. The page retains only the primitive-valued subscriptions it needs for delete menu item configuration (canDowngrade,amountOwed,isLoadingBill, owned-policy counts via a stable selector).Fixed Issues
$ #91175
PROPOSAL:
Tests
Go to Settings → Workspaces (the workspaces list page).
Open the three-dots menu for any workspace you own and tap Delete.
Verify the delete confirmation modal appears and the workspace is removed after confirming.
With a paid workspace as the only active owned workspace, open the three-dots menu and tap Delete.
Verify the bill calculation flow runs (loading spinner appears on the delete item) and the confirmation modal opens correctly after the bill is calculated.
Open the three-dots menu for a workspace you don't own.
Verify the Delete option is not present and the Leave/Transfer options still appear and work normally.
Offline tests
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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