diff --git a/mobile/src/source-control/MobileSourceControlCreatePrEntry.tsx b/mobile/src/source-control/MobileSourceControlCreatePrEntry.tsx index c58838e68..b60c63217 100644 --- a/mobile/src/source-control/MobileSourceControlCreatePrEntry.tsx +++ b/mobile/src/source-control/MobileSourceControlCreatePrEntry.tsx @@ -13,6 +13,7 @@ export function MobileSourceControlCreatePrEntry({ action }: Props) { return null } const enabled = !action.disabled + const copy = action.hint ?? action.label return ( )} - - {action.label} + + {copy} - {action.hint ? ( - - {action.hint} - - ) : null} ) } diff --git a/mobile/src/source-control/mobile-create-pr-action.test.ts b/mobile/src/source-control/mobile-create-pr-action.test.ts index 99b86cc85..c665b034f 100644 --- a/mobile/src/source-control/mobile-create-pr-action.test.ts +++ b/mobile/src/source-control/mobile-create-pr-action.test.ts @@ -4,7 +4,10 @@ import type { HostedReviewCreationEligibility, HostedReviewProvider } from '../../../src/shared/hosted-review' -import { buildMobileCreatePrAction } from './mobile-create-pr-action' +import { + buildMobileCreatePrAction, + type MobileCreatePrEligibilityState +} from './mobile-create-pr-action' function eligibility( overrides: Partial = {} @@ -108,26 +111,21 @@ describe('buildMobileCreatePrAction', () => { 'unsupported_provider', 'detached_head', null - ])('hides %s', (blockedReason) => { + ])('keeps a disabled status row for %s', (blockedReason) => { const { descriptor } = action({ eligibility: eligibility({ canCreate: false, blockedReason }) }) - expect(descriptor.visible).toBe(false) + expect(descriptor).toMatchObject({ visible: true, disabled: true }) + expect(descriptor.hint).toBeTruthy() }) - it('hides cold loading, errors, and missing branches', () => { - const onCreatePr = vi.fn() - - expect( - buildMobileCreatePrAction({ - branch: 'feature', - eligibilityState: { kind: 'loading', eligibility: null }, - busyAction: null, - onCreatePr - }).visible - ).toBe(false) - expect(action({ eligibility: null }).descriptor.visible).toBe(false) + it('keeps an unavailable row after errors but hides missing branches', () => { + expect(action({ eligibility: null }).descriptor).toMatchObject({ + visible: true, + disabled: true, + label: 'Review status unavailable' + }) expect(action({ branch: null }).descriptor.visible).toBe(false) }) @@ -146,12 +144,16 @@ describe('buildMobileCreatePrAction', () => { expect(onCreatePr).not.toHaveBeenCalled() }) - it('hides any provider that does not support hosted-review creation', () => { + it('keeps a disabled status row for providers without review creation', () => { const { descriptor } = action({ eligibility: eligibility({ provider: 'bitbucket' as HostedReviewProvider }) }) - expect(descriptor.visible).toBe(false) + expect(descriptor).toMatchObject({ + visible: true, + disabled: true, + label: 'Review creation unavailable for this provider' + }) }) it('keeps a creatable button visible but disabled while a newer eligibility loads', () => { @@ -167,3 +169,76 @@ describe('buildMobileCreatePrAction', () => { expect(descriptor).toMatchObject({ visible: true, disabled: true, loading: false }) }) }) + +// Issue #8411: the Create PR entry renders directly above the Stage All row and +// the changed-files list, so appearing late shifts the list down by +// createPrBlock marginTop (12) + createPrButton height (42) = 54pt while the +// user is already reading it. The row must keep its footprint across the cold +// eligibility fetch. +describe('cold-mount layout stability (issue #8411)', () => { + // useMobileHostedReviewEligibility's real cold-mount sequence for a creatable + // branch: no prior snapshot exists, so `loading` carries eligibility: null. + const coldMount: MobileCreatePrEligibilityState[] = [ + { kind: 'loading', eligibility: null }, + { kind: 'ready', eligibility: eligibility({ canCreate: true }) } + ] + + it('reserves the button row while the first eligibility request is in flight', () => { + const descriptors = coldMount.map((eligibilityState) => + buildMobileCreatePrAction({ + branch: 'feature', + eligibilityState, + busyAction: null, + onCreatePr: vi.fn() + }) + ) + + const footprint = descriptors.map(({ visible }) => visible) + + // On the buggy parent this was [false, true] -- the false -> true step is the jump. + expect(footprint).toEqual([true, true]) + }) + + it.each([ + [ + 'existing review', + { + kind: 'ready', + eligibility: eligibility({ canCreate: false, blockedReason: 'existing_review' }) + } satisfies MobileCreatePrEligibilityState + ], + [ + 'unsupported provider', + { + kind: 'ready', + eligibility: eligibility({ provider: 'bitbucket' as HostedReviewProvider }) + } satisfies MobileCreatePrEligibilityState + ], + ['eligibility error', { kind: 'error' } satisfies MobileCreatePrEligibilityState] + ])('keeps the cold-load footprint when %s resolves', (_label, resolvedState) => { + const footprint = [coldMount[0], resolvedState].map((eligibilityState) => { + const descriptor = buildMobileCreatePrAction({ + branch: 'feature', + eligibilityState, + busyAction: null, + onCreatePr: vi.fn() + }) + return descriptor.visible + }) + + expect(footprint).toEqual([true, true]) + }) + + it('does not offer a tappable action against unresolved eligibility', () => { + const descriptor = buildMobileCreatePrAction({ + branch: 'feature', + eligibilityState: { kind: 'loading', eligibility: null }, + busyAction: null, + onCreatePr: vi.fn() + }) + + // Reserving space must not make the placeholder actionable: there is no + // canCreate/pushFirst answer yet, so a tap has nothing correct to do. + expect(descriptor.disabled).toBe(true) + }) +}) diff --git a/mobile/src/source-control/mobile-create-pr-action.ts b/mobile/src/source-control/mobile-create-pr-action.ts index 0636fc015..fe75cc60a 100644 --- a/mobile/src/source-control/mobile-create-pr-action.ts +++ b/mobile/src/source-control/mobile-create-pr-action.ts @@ -1,7 +1,4 @@ -import type { - HostedReviewCreationBlockedReason, - HostedReviewCreationEligibility -} from '../../../src/shared/hosted-review' +import type { HostedReviewCreationEligibility } from '../../../src/shared/hosted-review' import { supportsHostedReviewCreation } from '../../../src/shared/hosted-review-creation-providers' import { hostedReviewCopy } from './hosted-review-copy' import { getMobilePrCreateBlockMessage } from './mobile-pr-create' @@ -29,13 +26,6 @@ export type BuildMobileCreatePrActionArgs = { onCreatePr: (pushFirst: boolean) => void } -const HIDDEN_BLOCKED_REASONS = new Set([ - 'detached_head', - 'existing_review', - 'unsupported_provider', - null -]) - const BUSY_ACTIONS = new Set(['create-pr', 'push-create-pr']) function hiddenAction(onPress: () => void): MobileCreatePrAction { @@ -56,18 +46,42 @@ export function buildMobileCreatePrAction({ onCreatePr }: BuildMobileCreatePrActionArgs): MobileCreatePrAction { const noop = () => {} - if (!branch || eligibilityState.kind === 'idle' || eligibilityState.kind === 'error') { + if (!branch || eligibilityState.kind === 'idle') { return hiddenAction(noop) } + if (eligibilityState.kind === 'error') { + return { + visible: true, + label: 'Review status unavailable', + disabled: true, + loading: false, + pushFirst: false, + onPress: noop + } + } const eligibility = eligibilityState.eligibility if (!eligibility) { - return hiddenAction(noop) + return { + visible: true, + label: 'Checking review status…', + disabled: true, + loading: true, + pushFirst: false, + onPress: noop + } } // Why: mirror desktop's structural provider gate (supportsHostedReviewCreation) // instead of relying on the host always emitting a hidden blockedReason for // non-creatable providers like bitbucket. if (!supportsHostedReviewCreation(eligibility.provider)) { - return hiddenAction(noop) + return { + visible: true, + label: 'Review creation unavailable for this provider', + disabled: true, + loading: false, + pushFirst: false, + onPress: noop + } } const copy = hostedReviewCopy(eligibility.provider) const label = `Create ${copy.titleLabel}` @@ -95,10 +109,6 @@ export function buildMobileCreatePrAction({ } } - if (HIDDEN_BLOCKED_REASONS.has(eligibility.blockedReason)) { - return hiddenAction(noop) - } - const hint = getMobilePrCreateBlockMessage({ provider: eligibility.provider, diff --git a/mobile/src/source-control/mobile-source-control-styles.ts b/mobile/src/source-control/mobile-source-control-styles.ts index d8dc20a14..fe8d8da0a 100644 --- a/mobile/src/source-control/mobile-source-control-styles.ts +++ b/mobile/src/source-control/mobile-source-control-styles.ts @@ -207,11 +207,10 @@ const baseStyles = StyleSheet.create({ fontWeight: '600' }, createPrBlock: { - marginTop: spacing.md, - gap: spacing.xs + marginTop: spacing.md }, createPrButton: { - minHeight: 42, + height: 42, borderRadius: radii.button, backgroundColor: colors.textPrimary, alignItems: 'center', @@ -236,10 +235,12 @@ const baseStyles = StyleSheet.create({ createPrButtonTextDisabled: { color: colors.textSecondary }, - createPrHint: { - color: colors.textMuted, + createPrButtonHint: { fontSize: typography.metaSize, - lineHeight: 16 + fontWeight: '600', + lineHeight: 16, + textAlign: 'center', + flexShrink: 1 } }) diff --git a/mobile/src/source-control/use-mobile-hosted-review-eligibility.test.ts b/mobile/src/source-control/use-mobile-hosted-review-eligibility.test.ts index cc1f4965d..13109a610 100644 --- a/mobile/src/source-control/use-mobile-hosted-review-eligibility.test.ts +++ b/mobile/src/source-control/use-mobile-hosted-review-eligibility.test.ts @@ -1,10 +1,48 @@ -import { describe, expect, it } from 'vitest' +import { createElement } from 'react' +import { act, create, type ReactTestRenderer } from 'react-test-renderer' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { HostedReviewCreationEligibility } from '../../../src/shared/hosted-review' import { - acceptsMobileHostedReviewEligibilityLoad, buildMobileHostedReviewEligibilityLoadKey, eligibilityStateAfterMobileHostedReviewError, - shouldFetchMobileHostedReviewEligibility + renderedMobileHostedReviewEligibilityState, + shouldFetchMobileHostedReviewEligibility, + useMobileHostedReviewEligibility, + type MobileHostedReviewEligibilityLoadKey, + type MobileHostedReviewEligibilityLoadSnapshot } from './use-mobile-hosted-review-eligibility' +import type { MobileCreatePrEligibilityState } from './mobile-create-pr-action' + +function eligibility( + overrides: Partial = {} +): HostedReviewCreationEligibility { + return { + provider: 'github', + review: null, + canCreate: true, + blockedReason: null, + nextAction: null, + defaultBaseRef: 'main', + title: 'feature', + body: '', + ...overrides + } +} + +function deferred(): { promise: Promise; resolve: (value: T) => void } { + let resolve!: (value: T) => void + const promise = new Promise((resolvePromise) => { + resolve = resolvePromise + }) + return { promise, resolve } +} + +function loadSnapshot( + key: MobileHostedReviewEligibilityLoadKey, + state: MobileCreatePrEligibilityState +): MobileHostedReviewEligibilityLoadSnapshot { + return { key, state } +} describe('mobile hosted review eligibility loader core', () => { it('does not fetch while disconnected or detached', () => { @@ -24,8 +62,9 @@ describe('mobile hosted review eligibility loader core', () => { ).toBe(false) }) - it('accepts only the latest generation for the current worktree branch identity', () => { + it('builds distinct keys for different branches', () => { const first = buildMobileHostedReviewEligibilityLoadKey({ + hostId: 'host-1', worktreeId: 'wt-1', branch: 'feature-a', hasUpstream: true, @@ -34,6 +73,7 @@ describe('mobile hosted review eligibility loader core', () => { hasUncommittedChanges: false }) const second = buildMobileHostedReviewEligibilityLoadKey({ + hostId: 'host-1', worktreeId: 'wt-1', branch: 'feature-b', hasUpstream: true, @@ -42,17 +82,281 @@ describe('mobile hosted review eligibility loader core', () => { hasUncommittedChanges: false }) + expect(first.identity).not.toBe(second.identity) + expect(first.fetch).not.toBe(second.fetch) + }) + + it('scopes load identity to the paired host', () => { + const input = { + worktreeId: 'repo-1::/workspace', + branch: 'feature', + hasUpstream: true, + ahead: 0, + behind: 0, + hasUncommittedChanges: false + } + const local = buildMobileHostedReviewEligibilityLoadKey({ ...input, hostId: 'local' }) + const ssh = buildMobileHostedReviewEligibilityLoadKey({ ...input, hostId: 'ssh-builder' }) + + expect(local.identity).not.toBe(ssh.identity) + expect(local.fetch).not.toBe(ssh.fetch) + }) + + it('renders a superseded same-identity snapshot as loading', () => { + const input = { + hostId: 'host-1', + worktreeId: 'wt-1', + branch: 'feature', + hasUpstream: true, + ahead: 0, + behind: 0 + } + const older = buildMobileHostedReviewEligibilityLoadKey({ + ...input, + hasUncommittedChanges: false + }) + const newest = buildMobileHostedReviewEligibilityLoadKey({ + ...input, + hasUncommittedChanges: true + }) + + expect(older.identity).toBe(newest.identity) expect( - acceptsMobileHostedReviewEligibilityLoad({ - generation: 1, - currentGeneration: 2, - identity: first.identity, - currentIdentity: second.identity + renderedMobileHostedReviewEligibilityState({ + snapshot: loadSnapshot(older, { kind: 'ready', eligibility: eligibility() }), + key: newest, + shouldFetch: true }) - ).toBe(false) + ).toMatchObject({ kind: 'loading', eligibility: eligibility() }) }) it('fails closed after errors', () => { expect(eligibilityStateAfterMobileHostedReviewError()).toEqual({ kind: 'error' }) }) }) + +// #8411: what the hook returns is what paints. A fetch-imminent frame must not +// render as `idle` (hidden row) or the Create PR row pops in a frame later. +describe('rendered eligibility state', () => { + it('renders fetch-imminent idle as an in-flight load', () => { + const key = buildMobileHostedReviewEligibilityLoadKey({ + hostId: 'host-1', + worktreeId: 'wt-1', + branch: 'feature', + hasUpstream: true, + ahead: 0, + behind: 0, + hasUncommittedChanges: false + }) + expect( + renderedMobileHostedReviewEligibilityState({ + snapshot: loadSnapshot(key, { kind: 'idle' }), + key, + shouldFetch: true + }) + ).toEqual({ kind: 'loading', eligibility: null }) + }) + + it('passes resolved and refetch states through untouched', () => { + const key = buildMobileHostedReviewEligibilityLoadKey({ + hostId: 'host-1', + worktreeId: 'wt-1', + branch: 'feature', + hasUpstream: true, + ahead: 0, + behind: 0, + hasUncommittedChanges: false + }) + const ready = { kind: 'ready', eligibility: eligibility() } as const + const refetch = { kind: 'loading', eligibility: eligibility() } as const + const error = { kind: 'error' } as const + + for (const state of [ready, refetch, error]) { + expect( + renderedMobileHostedReviewEligibilityState({ + snapshot: loadSnapshot(key, state), + key, + shouldFetch: true + }) + ).toBe(state) + } + }) + + it('renders idle when a fetch is not possible, hiding stale snapshots in the same render', () => { + const key = buildMobileHostedReviewEligibilityLoadKey({ + hostId: 'host-1', + worktreeId: 'wt-1', + branch: 'feature', + hasUpstream: true, + ahead: 0, + behind: 0, + hasUncommittedChanges: false + }) + expect( + renderedMobileHostedReviewEligibilityState({ + snapshot: loadSnapshot(key, { kind: 'ready', eligibility: eligibility() }), + key, + shouldFetch: false + }) + ).toEqual({ kind: 'idle' }) + }) +}) + +describe('eligibility request ordering', () => { + let renderer: ReactTestRenderer | null = null + let renderedState: MobileCreatePrEligibilityState = { kind: 'idle' } + + beforeEach(() => { + globalThis.IS_REACT_ACT_ENVIRONMENT = true + }) + + afterEach(() => { + act(() => renderer?.unmount()) + renderer = null + }) + + it('does not let an older response overwrite the newest state', async () => { + type Response = { ok: true; result: HostedReviewCreationEligibility } + const requests: ReturnType>[] = [] + const client = { + sendRequest: vi.fn(() => { + const request = deferred() + requests.push(request) + return request.promise + }) + } + const newest = eligibility({ canCreate: false, blockedReason: 'existing_review' }) + const older = eligibility() + + function Harness({ dirty }: { dirty: boolean }): null { + renderedState = useMobileHostedReviewEligibility({ + client: client as never, + connState: 'connected', + hostId: 'host-1', + worktreeId: 'wt-1', + branch: 'feature', + hasUpstream: true, + ahead: 0, + behind: 0, + hasUncommittedChanges: dirty + }) + return null + } + + await act(async () => { + renderer = create(createElement(Harness, { dirty: false })) + await Promise.resolve() + }) + await act(async () => { + renderer?.update(createElement(Harness, { dirty: true })) + await Promise.resolve() + }) + expect(requests).toHaveLength(2) + + await act(async () => { + requests[1]!.resolve({ ok: true, result: newest }) + await Promise.resolve() + }) + await act(async () => { + requests[0]!.resolve({ ok: true, result: older }) + await Promise.resolve() + }) + + expect(renderedState).toMatchObject({ kind: 'ready', eligibility: newest }) + }) + + it('disables a ready snapshot in the render that changes its fetch key', async () => { + type Response = { ok: true; result: HostedReviewCreationEligibility } + const requests: ReturnType>[] = [] + const client = { + sendRequest: vi.fn(() => { + const request = deferred() + requests.push(request) + return request.promise + }) + } + const ready = eligibility() + + function Harness({ dirty }: { dirty: boolean }): null { + renderedState = useMobileHostedReviewEligibility({ + client: client as never, + connState: 'connected', + hostId: 'host-1', + worktreeId: 'wt-1', + branch: 'feature', + hasUpstream: true, + ahead: 0, + behind: 0, + hasUncommittedChanges: dirty + }) + return null + } + + await act(async () => { + renderer = create(createElement(Harness, { dirty: false })) + await Promise.resolve() + }) + await act(async () => { + requests[0]!.resolve({ ok: true, result: ready }) + await Promise.resolve() + }) + expect(renderedState).toMatchObject({ kind: 'ready', eligibility: ready }) + + await act(async () => { + renderer?.update(createElement(Harness, { dirty: true })) + await Promise.resolve() + }) + expect(renderedState).toMatchObject({ kind: 'loading', eligibility: ready }) + }) + + it('invalidates a request when its hook instance unmounts', async () => { + type Response = { ok: true; result: HostedReviewCreationEligibility } + const requests: ReturnType>[] = [] + const client = { + sendRequest: vi.fn(() => { + const request = deferred() + requests.push(request) + return request.promise + }) + } + const newest = eligibility({ canCreate: false, blockedReason: 'existing_review' }) + + function Harness(): null { + renderedState = useMobileHostedReviewEligibility({ + client: client as never, + connState: 'connected', + hostId: 'host-1', + worktreeId: 'wt-1', + branch: 'feature', + hasUpstream: true, + ahead: 0, + behind: 0, + hasUncommittedChanges: false + }) + return null + } + + await act(async () => { + renderer = create(createElement(Harness)) + await Promise.resolve() + }) + act(() => renderer?.unmount()) + renderer = null + await act(async () => { + renderer = create(createElement(Harness)) + await Promise.resolve() + }) + expect(requests).toHaveLength(2) + + await act(async () => { + requests[1]!.resolve({ ok: true, result: newest }) + await Promise.resolve() + }) + await act(async () => { + requests[0]!.resolve({ ok: true, result: eligibility() }) + await Promise.resolve() + }) + + expect(renderedState).toMatchObject({ kind: 'ready', eligibility: newest }) + }) +}) diff --git a/mobile/src/source-control/use-mobile-hosted-review-eligibility.ts b/mobile/src/source-control/use-mobile-hosted-review-eligibility.ts index ff5a98c43..e229d4953 100644 --- a/mobile/src/source-control/use-mobile-hosted-review-eligibility.ts +++ b/mobile/src/source-control/use-mobile-hosted-review-eligibility.ts @@ -1,4 +1,4 @@ -import { useEffect, useRef, useState } from 'react' +import { useEffect, useState } from 'react' import type { HostedReviewCreationEligibility } from '../../../src/shared/hosted-review' import type { RpcClient } from '../transport/rpc-client' import type { ConnectionState } from '../transport/types' @@ -11,6 +11,7 @@ import type { MobileCreatePrEligibilityState } from './mobile-create-pr-action' export type MobileHostedReviewEligibilityLoaderInput = { client: RpcClient | null connState: ConnectionState + hostId: string worktreeId: string branch: string | null | undefined hasUpstream: boolean | undefined @@ -24,13 +25,19 @@ export type MobileHostedReviewEligibilityLoadKey = { fetch: string } +export type MobileHostedReviewEligibilityLoadSnapshot = { + key: MobileHostedReviewEligibilityLoadKey + state: MobileCreatePrEligibilityState +} + export function buildMobileHostedReviewEligibilityLoadKey( input: Omit ): MobileHostedReviewEligibilityLoadKey { const branch = input.branch ?? '' return { - identity: `${input.worktreeId}\0${branch}`, + identity: `${input.hostId}\0${input.worktreeId}\0${branch}`, fetch: [ + input.hostId, input.worktreeId, branch, String(input.hasUpstream ?? ''), @@ -47,25 +54,42 @@ export function shouldFetchMobileHostedReviewEligibility( return input.connState === 'connected' && input.client !== null && !!input.branch } -export function acceptsMobileHostedReviewEligibilityLoad(args: { - generation: number - currentGeneration: number - identity: string - currentIdentity: string -}): boolean { - return args.generation === args.currentGeneration && args.identity === args.currentIdentity -} - export function eligibilityStateAfterMobileHostedReviewError(): MobileCreatePrEligibilityState { return { kind: 'error' } } +export function renderedMobileHostedReviewEligibilityState(args: { + snapshot: MobileHostedReviewEligibilityLoadSnapshot + key: MobileHostedReviewEligibilityLoadKey + shouldFetch: boolean +}): MobileCreatePrEligibilityState { + if (!args.shouldFetch) { + return { kind: 'idle' } + } + const { snapshot, key } = args + if (snapshot.key.identity !== key.identity) { + return { kind: 'loading', eligibility: null } + } + const { state } = snapshot + if (snapshot.key.fetch !== key.fetch) { + return { + kind: 'loading', + eligibility: state.kind === 'ready' || state.kind === 'loading' ? state.eligibility : null + } + } + if (state.kind === 'idle') { + return { kind: 'loading', eligibility: null } + } + return state +} + export function useMobileHostedReviewEligibility( input: MobileHostedReviewEligibilityLoaderInput ): MobileCreatePrEligibilityState { const { client, connState, + hostId, worktreeId, branch, hasUpstream, @@ -74,11 +98,8 @@ export function useMobileHostedReviewEligibility( hasUncommittedChanges } = input const shouldFetch = shouldFetchMobileHostedReviewEligibility({ client, connState, branch }) - const [state, setState] = useState({ kind: 'idle' }) - const generationRef = useRef(0) - const currentIdentityRef = useRef('') - const lastResetIdentityRef = useRef('') const key = buildMobileHostedReviewEligibilityLoadKey({ + hostId, worktreeId, branch, hasUpstream, @@ -86,38 +107,35 @@ export function useMobileHostedReviewEligibility( behind, hasUncommittedChanges }) - - if (lastResetIdentityRef.current !== key.identity) { - lastResetIdentityRef.current = key.identity - setState({ kind: 'idle' }) - } - currentIdentityRef.current = key.identity + const [snapshot, setSnapshot] = useState({ + key: { identity: '', fetch: '' }, + state: { kind: 'idle' } + }) useEffect(() => { - const generation = generationRef.current + 1 - generationRef.current = generation - const isCurrent = () => - acceptsMobileHostedReviewEligibilityLoad({ - generation, - currentGeneration: generationRef.current, - identity: key.identity, - currentIdentity: currentIdentityRef.current - }) + let active = true if (!shouldFetch) { - if (isCurrent()) { - setState({ kind: 'idle' }) + setSnapshot({ key, state: { kind: 'idle' } }) + return () => { + active = false } - return } if (!client || !branch) { - return + return () => { + active = false + } } - setState((prev) => ({ - kind: 'loading', - eligibility: prev.kind === 'ready' ? prev.eligibility : null - })) + setSnapshot((previous) => { + const previousState = previous.state + const eligibility = + previous.key.identity === key.identity && + (previousState.kind === 'ready' || previousState.kind === 'loading') + ? previousState.eligibility + : null + return { key, state: { kind: 'loading', eligibility } } + }) const requestInput: MobileHostedReviewEligibilityInput = { branch, hasUncommittedChanges, @@ -127,20 +145,23 @@ export function useMobileHostedReviewEligibility( } void fetchMobileHostedReviewEligibility(client, worktreeId, requestInput) .then((eligibility: HostedReviewCreationEligibility | null) => { - if (!isCurrent()) { + if (!active) { return } if (!eligibility) { - setState({ kind: 'error' }) + setSnapshot({ key, state: eligibilityStateAfterMobileHostedReviewError() }) return } - setState({ kind: 'ready', eligibility }) + setSnapshot({ key, state: { kind: 'ready', eligibility } }) }) .catch(() => { - if (isCurrent()) { - setState(eligibilityStateAfterMobileHostedReviewError()) + if (active) { + setSnapshot({ key, state: eligibilityStateAfterMobileHostedReviewError() }) } }) + return () => { + active = false + } }, [ ahead, behind, @@ -149,6 +170,7 @@ export function useMobileHostedReviewEligibility( connState, hasUncommittedChanges, hasUpstream, + hostId, key.fetch, key.identity, shouldFetch, @@ -159,5 +181,9 @@ export function useMobileHostedReviewEligibility( // snapshot in the same render, before the effect posts `idle` — otherwise the // Create PR button could stay enabled for one paint after the worktree is // no longer fetchable. - return shouldFetch ? state : { kind: 'idle' } + return renderedMobileHostedReviewEligibilityState({ + snapshot, + key, + shouldFetch + }) } diff --git a/mobile/src/source-control/use-mobile-source-control-create-pr-action.ts b/mobile/src/source-control/use-mobile-source-control-create-pr-action.ts index 44655d147..49ec9bda8 100644 --- a/mobile/src/source-control/use-mobile-source-control-create-pr-action.ts +++ b/mobile/src/source-control/use-mobile-source-control-create-pr-action.ts @@ -6,6 +6,7 @@ import type { MobileGitStatusResult } from './mobile-git-status' type Params = { client: Parameters[0]['client'] connState: Parameters[0]['connState'] + hostId: string worktreeId: string status: MobileGitStatusResult | null hasUncommittedChanges: boolean @@ -16,6 +17,7 @@ type Params = { export function useMobileSourceControlCreatePrAction({ client, connState, + hostId, worktreeId, status, hasUncommittedChanges, @@ -26,6 +28,7 @@ export function useMobileSourceControlCreatePrAction({ const eligibilityState = useMobileHostedReviewEligibility({ client, connState, + hostId, worktreeId, branch: status?.branch, hasUpstream: upstream?.hasUpstream, diff --git a/mobile/src/source-control/use-mobile-source-control-state.ts b/mobile/src/source-control/use-mobile-source-control-state.ts index e50525f3c..1f541739d 100644 --- a/mobile/src/source-control/use-mobile-source-control-state.ts +++ b/mobile/src/source-control/use-mobile-source-control-state.ts @@ -209,6 +209,7 @@ export function useMobileSourceControlState(params: MobileSourceControlStatePara const createPrAction = useMobileSourceControlCreatePrAction({ client, connState, + hostId, worktreeId, status, hasUncommittedChanges: entries.length > 0,