Workspace - Deleting a Workspace while offline crashes the app

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.4.7-0
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

1. Access staging.new.expensify.com
2. Sign into any account
3. Turn off internet
4. Go to Profile > Workspace > 3 dot menu in top right corner > Delete

Expected Result:

User expects to be able to delete a WS while offline and the WS be crossed out

Actual Result:

The app crashes

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6297608_1701450499040.Deleting_WS_offline_crashes_the_app_.mp4

View all open jobs on GitHub

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

Triggered auto assignment to @tylerkaraszewski (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[bernhardoj]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Deleting a workspace while offline crashes the app.

What is the root cause of that problem?

This is a regression from #31518. When we delete a workspace while offline, every text in the workspace page is strikethrough-ed, thanks to OfflineWithFeedback.

App/src/pages/workspace/WorkspaceInitialPage.js

Lines 259 to 325 in 1682be4

	<OfflineWithFeedback
	pendingAction={policy.pendingAction}
	onClose={() => dismissError(policy.id)}
	errors={policy.errors}
	errorRowStyles={[styles.ph5, styles.pv2]}
	>
	<View style={[styles.flex1]}>
	<View style={styles.avatarSectionWrapper}>
	<View style={[styles.settingsPageBody, styles.alignItemsCenter]}>
	<Tooltip text={translate('workspace.common.settings')}>
	<PressableWithoutFeedback
	disabled={hasPolicyCreationError || isExecuting}
	style={[styles.pRelative, styles.avatarLarge]}
	onPress={singleExecution(waitForNavigate(() => openEditor(policy.id)))}
	accessibilityLabel={translate('workspace.common.settings')}
	role={CONST.ACCESSIBILITY_ROLE.BUTTON}
	>
	<Avatar
	containerStyles={styles.avatarLarge}
	imageStyles={[styles.avatarLarge, styles.alignSelfCenter]}
	source={policy.avatar ? policy.avatar : ReportUtils.getDefaultWorkspaceAvatar(policyName)}
	fallbackIcon={Expensicons.FallbackWorkspaceAvatar}
	size={CONST.AVATAR_SIZE.LARGE}
	name={policyName}
	type={CONST.ICON_TYPE_WORKSPACE}
	/>
	</PressableWithoutFeedback>
	</Tooltip>
	{!_.isEmpty(policy.name) && (
	<Tooltip text={translate('workspace.common.settings')}>
	<PressableWithoutFeedback
	disabled={hasPolicyCreationError || isExecuting}
	style={[styles.alignSelfCenter, styles.mt4, styles.w100]}
	onPress={singleExecution(waitForNavigate(() => openEditor(policy.id)))}
	accessibilityLabel={translate('workspace.common.settings')}
	role={CONST.ACCESSIBILITY_ROLE.BUTTON}
	>
	<Text
	numberOfLines={1}
	style={[styles.textHeadline, styles.alignSelfCenter, styles.pre]}
	>
	{policy.name}
	</Text>
	</PressableWithoutFeedback>
	</Tooltip>
	)}
	</View>
	</View>
	{/*
	Ideally we should use MenuList component for MenuItems with singleExecution/Navigation actions.
	In this case where user can click on workspace avatar or menu items, we need to have a check for `isExecuting`. So, we are directly mapping menuItems.
	*/}
	{_.map(menuItems, (item) => (
	<MenuItem
	key={item.translationKey}
	disabled={hasPolicyCreationError || isExecuting}
	interactive={!hasPolicyCreationError}
	title={translate(item.translationKey)}
	icon={item.icon}
	iconRight={item.iconRight}
	onPress={item.action}
	shouldShowRightIcon
	brickRoadIndicator={item.brickRoadIndicator}
	/>
	))}
	</View>
	</OfflineWithFeedback>

OfflineWithFeedback will clone all children and apply the strikethrough style and the result of the clone will be in an array because we are using map here.

App/src/components/OfflineWithFeedback.js

Lines 83 to 92 in 1682be4

	return React.Children.map(children, (child) => {
	if (!React.isValidElement(child)) {
	return child;
	}
	const props = {style: StyleUtils.combineStyles(child.props.style, styles.offlineFeedback.deleted, styles.userSelectNone)};
	if (child.props.children) {
	props.children = applyStrikeThrough(child.props.children, styles);
	}
	return React.cloneElement(child, props);
	});

The error itself comes from the Tooltip > Hoverable (workspace settings tooltip) component because it didn't handle children as an array correctly.

After the OfflineWithFeedback cloning, the tooltip children is changed:
from PressableWithoutFeedback to [PressableWithoutFeedback]

In Hoverable, we clone the children with React.cloneElement.

App/src/components/Hoverable/ActiveHoverable.tsx

Line 86 in 1682be4

const child = useMemo(() => (typeof children === 'function' ? children(!isScrollingRef.current && isHovered) : children), [children, isHovered]);

App/src/components/Hoverable/ActiveHoverable.tsx

Lines 129 to 134 in 1682be4

	return cloneElement(child, {
	ref: hijackRef,
	onMouseEnter: hoverAndForwardOnMouseEnter,
	onMouseLeave: unhoverAndForwardOnMouseLeave,
	onBlur: unhoverAndForwardOnBlur,
	});

But React.cloneElement can't accept an array, so it throws an error.

we had the same old issue before here

What changes do you think we should make in order to solve the problem?

Previously, we handled the children as an array correctly like this:

We previously used React.Children.only to make sure the children only contain 1 element. Then, if it's an array, we take the first element.

So, the solution is to use the old code back again.

[bernhardoj]

Based on this comment, looks like we removed mapChildren to support multiple children which isn't possible with cloneElement.

In the old tooltip, we have a prop called absolute to decide whether to wrap the tooltip children with View or not, which is removed in #16052.

If there is a case of a tooltip that has multiple children, then we should wrap it with a View inside the children, not in the Tooltip implementation. If not, it will throw an error.

<Tooltip>
    <View>
        <View />
        <View />
    </View>
<Tooltip>

[situchan]

@tylerkaraszewski I think it's better to revert #31518 and let them rework considering this regression as it was just component refactor, neither bug nor feature.

[yuwenmemon]

Revert was CP'ed to staging. Closing this out!