From 29be0cc2e032ef03b93ebda9c03809bf9f488c1f Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 28 Aug 2026 04:22:51 -0700 Subject: [PATCH] fix(mobile): keep the create form when a render is thrown away MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The session key was built from counters mutated during render. A blurred screen suspends this subtree (react-native-screens freezes via react-freeze), so React runs the component and then discards that render — but the counter bumps survive it. The next committed render then produced a new key and remounted the form, throwing away the picked source for a host switch or a close that never committed. Hold the open epoch in state, which React discards with the render that set it, and put the host in the key directly instead of counting host changes. --- .../src/components/NewWorktreeModal.test.tsx | 80 ++++++++++++++++++- mobile/src/components/NewWorktreeModal.tsx | 30 +++---- 2 files changed, 91 insertions(+), 19 deletions(-) diff --git a/mobile/src/components/NewWorktreeModal.test.tsx b/mobile/src/components/NewWorktreeModal.test.tsx index 8c48f35261c..23aabfbff21 100644 --- a/mobile/src/components/NewWorktreeModal.test.tsx +++ b/mobile/src/components/NewWorktreeModal.test.tsx @@ -1,4 +1,4 @@ -import { createElement } from 'react' +import { createElement, Suspense, type ReactElement } from 'react' import { act, create, type ReactTestRenderer } from 'react-test-renderer' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { RpcClient } from '../transport/rpc-client' @@ -420,6 +420,84 @@ describe('NewWorktreeModal project targets', () => { ]) }) + // react-native-screens freezes a blurred screen by suspending its subtree + // (react-freeze), so React runs this component and then throws that render away. + // Anything the render mutated in place outlives the work React discarded. + function suspendableTree(frozen: () => boolean, child: ReactElement) { + const never = new Promise(() => {}) + const Freezer = () => { + if (frozen()) { + throw never + } + return null + } + // The modal renders first, so its render completes before the freeze throws. + return createElement(Suspense, { fallback: null }, child, createElement(Freezer, null)) + } + + it('keeps the form when a host switch renders but never commits', async () => { + setCachedRepos('host-2', repos) + const client = { + sendRequest: vi.fn().mockImplementation(() => new Promise(() => {})) + } as unknown as RpcClient + const modalProps = { visible: true, client, onCreated: () => {}, onClose: () => {} } + let frozen = false + const tree = (hostId: string) => + suspendableTree(() => frozen, createElement(NewWorktreeModal, { ...modalProps, hostId })) + + await act(async () => { + renderer = create(tree('host-1')) + }) + act(() => sourceInputs(renderer)[0]!.props.onChangeText('keep-me')) + + frozen = true + await act(async () => { + renderer.update(tree('host-2')) + }) + frozen = false + await act(async () => { + renderer.update(tree('host-1')) + }) + + // host-2 never committed, so host-1's session was never superseded. + expect(sourceInputs(renderer).map((input) => input.props.value)).toEqual(['keep-me', 'keep-me']) + }) + + it('keeps the form when a close renders but never commits', async () => { + const client = { + sendRequest: vi.fn().mockImplementation(() => new Promise(() => {})) + } as unknown as RpcClient + const modalProps = { + client, + hostId: 'host-1', + onCreated: () => {}, + onClose: () => {} + } + let frozen = false + const tree = (visible: boolean) => + suspendableTree(() => frozen, createElement(NewWorktreeModal, { ...modalProps, visible })) + + await act(async () => { + renderer = create(tree(true)) + }) + act(() => sourceInputs(renderer)[0]!.props.onChangeText('keep-me-too')) + + frozen = true + await act(async () => { + renderer.update(tree(false)) + }) + frozen = false + await act(async () => { + renderer.update(tree(true)) + }) + + // The drawer never committed a closed state, so this is not a reopening. + expect(sourceInputs(renderer).map((input) => input.props.value)).toEqual([ + 'keep-me-too', + 'keep-me-too' + ]) + }) + it('starts a fresh form session when the modal switches hosts', async () => { setCachedRepos('host-2', repos) const client = { diff --git a/mobile/src/components/NewWorktreeModal.tsx b/mobile/src/components/NewWorktreeModal.tsx index 7bbda7dd924..a986d347782 100644 --- a/mobile/src/components/NewWorktreeModal.tsx +++ b/mobile/src/components/NewWorktreeModal.tsx @@ -1,4 +1,4 @@ -import { useMemo, useRef, useState } from 'react' +import { useMemo, useState } from 'react' import { Keyboard } from 'react-native' import { getComposerRepoWorktreeBranches } from '../../../src/shared/composer-branch-selection' import { getProjectIdentityKey } from '../../../src/shared/project-host-setup-projection' @@ -33,31 +33,25 @@ import { useNewWorkspaceSetupScript } from './use-new-workspace-setup-script' import { useNewWorktreeDrawerNavigation } from './use-new-worktree-drawer-navigation' export function NewWorktreeModal(props: NewWorktreeModalProps) { - const openEpochRef = useRef(0) - const wasVisibleRef = useRef(false) - const hostEpochRef = useRef({ hostId: props.hostId, epoch: 0 }) - // Why: each drawer opening is a fresh form session; remounting resets local // form state before paint instead of clearing it in a visible-prop Effect. - if (props.visible && !wasVisibleRef.current) { - openEpochRef.current += 1 + // State, not a ref: react-native-screens freezes a blurred screen by suspending + // this subtree, and a counter bumped during a render React then throws away + // would restart the session for an opening that never committed. + const [session, setSession] = useState({ openEpoch: 0, visible: props.visible }) + if (session.visible !== props.visible) { + setSession({ + openEpoch: props.visible ? session.openEpoch + 1 : session.openEpoch, + visible: props.visible + }) } - wasVisibleRef.current = props.visible + // Why: key the session on the HOST, never on the RpcClient object. A reconnect, // forceReconnect, or foreground revival swaps that object for the same host // (see useHostClient), and keying on it silently remounted this form mid-edit // and threw away the picked source. Every client-scoped hook below already // drops responses from a superseded client, so no remount is needed for that. - if (hostEpochRef.current.hostId !== props.hostId) { - hostEpochRef.current = { hostId: props.hostId, epoch: hostEpochRef.current.epoch + 1 } - } - - return ( - - ) + return } function NewWorktreeModalContent(props: NewWorktreeModalProps) {