diff --git a/mobile/src/source-control/hosted-review-eligibility-memory.test.ts b/mobile/src/source-control/hosted-review-eligibility-memory.test.ts new file mode 100644 index 00000000000..1eb84f0b5cc --- /dev/null +++ b/mobile/src/source-control/hosted-review-eligibility-memory.test.ts @@ -0,0 +1,63 @@ +import { beforeEach, describe, expect, it } from 'vitest' +import type { HostedReviewCreationEligibility } from '../../../src/shared/hosted-review' +import { + forgetAllHostedReviewEligibility, + recallHostedReviewEligibility, + rememberHostedReviewEligibility +} from './hosted-review-eligibility-memory' + +function eligibility( + overrides: Partial = {} +): HostedReviewCreationEligibility { + return { + provider: 'github', + review: null, + canCreate: true, + blockedReason: null, + nextAction: null, + defaultBaseRef: 'main', + title: 'feature', + body: '', + ...overrides + } +} + +describe('hosted review eligibility memory', () => { + beforeEach(() => { + forgetAllHostedReviewEligibility() + }) + + it('recalls the last remembered answer per identity', () => { + const first = eligibility() + const blocked = eligibility({ canCreate: false, blockedReason: 'existing_review' }) + + rememberHostedReviewEligibility('wt-1\0feature', first) + rememberHostedReviewEligibility('wt-1\0other', blocked) + + expect(recallHostedReviewEligibility('wt-1\0feature')).toBe(first) + expect(recallHostedReviewEligibility('wt-1\0other')).toBe(blocked) + expect(recallHostedReviewEligibility('wt-2\0feature')).toBeNull() + }) + + it('overwrites an identity with its newest answer', () => { + const newest = eligibility({ canCreate: false, blockedReason: 'needs_push' }) + + rememberHostedReviewEligibility('wt-1\0feature', eligibility()) + rememberHostedReviewEligibility('wt-1\0feature', newest) + + expect(recallHostedReviewEligibility('wt-1\0feature')).toBe(newest) + }) + + it('evicts the least recently remembered identity beyond the bound', () => { + for (let i = 0; i < 64; i++) { + rememberHostedReviewEligibility(`wt-${i}`, eligibility()) + } + // Refresh wt-0 so wt-1 is now the oldest. + rememberHostedReviewEligibility('wt-0', eligibility()) + rememberHostedReviewEligibility('wt-64', eligibility()) + + expect(recallHostedReviewEligibility('wt-1')).toBeNull() + expect(recallHostedReviewEligibility('wt-0')).not.toBeNull() + expect(recallHostedReviewEligibility('wt-64')).not.toBeNull() + }) +}) diff --git a/mobile/src/source-control/hosted-review-eligibility-memory.ts b/mobile/src/source-control/hosted-review-eligibility-memory.ts new file mode 100644 index 00000000000..5ef48b47b7c --- /dev/null +++ b/mobile/src/source-control/hosted-review-eligibility-memory.ts @@ -0,0 +1,35 @@ +import type { HostedReviewCreationEligibility } from '../../../src/shared/hosted-review' + +// Why: every source-control open remounts the eligibility hook, so without a +// cross-mount memory each open cold-fetches and the Create PR row's footprint +// is unknown until the host answers — the row then pops in or collapses under +// the user's finger (#8411). Remembering the last resolved answer per +// worktree+branch lets the next open render the correct footprint immediately. +const MAX_REMEMBERED_ELIGIBILITIES = 64 + +const lastResolvedByIdentity = new Map() + +export function rememberHostedReviewEligibility( + identity: string, + eligibility: HostedReviewCreationEligibility +): void { + // Delete-then-set keeps insertion order as recency so eviction drops the LRU entry. + lastResolvedByIdentity.delete(identity) + lastResolvedByIdentity.set(identity, eligibility) + if (lastResolvedByIdentity.size > MAX_REMEMBERED_ELIGIBILITIES) { + const oldest = lastResolvedByIdentity.keys().next().value + if (oldest !== undefined) { + lastResolvedByIdentity.delete(oldest) + } + } +} + +export function recallHostedReviewEligibility( + identity: string +): HostedReviewCreationEligibility | null { + return lastResolvedByIdentity.get(identity) ?? null +} + +export function forgetAllHostedReviewEligibility(): void { + lastResolvedByIdentity.clear() +} 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 99b86cc85e7..9cea4b604e9 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 = {} @@ -116,17 +119,7 @@ describe('buildMobileCreatePrAction', () => { expect(descriptor.visible).toBe(false) }) - 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) + it('hides errors and missing branches', () => { expect(action({ eligibility: null }).descriptor.visible).toBe(false) expect(action({ branch: null }).descriptor.visible).toBe(false) }) @@ -167,3 +160,45 @@ 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 minHeight (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 footprint = coldMount.map( + (eligibilityState) => + buildMobileCreatePrAction({ + branch: 'feature', + eligibilityState, + busyAction: null, + onCreatePr: vi.fn() + }).visible + ) + + // On the buggy parent this was [false, true] -- the false -> true step is the jump. + 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 0636fc01588..fde757319df 100644 --- a/mobile/src/source-control/mobile-create-pr-action.ts +++ b/mobile/src/source-control/mobile-create-pr-action.ts @@ -61,7 +61,17 @@ export function buildMobileCreatePrAction({ } const eligibility = eligibilityState.eligibility if (!eligibility) { - return hiddenAction(noop) + // Cold loading: the first answer for this branch is still in flight. + // Reserve the row with a disabled placeholder instead of unmounting, so + // the changed-files list doesn't shift under the user when it lands (#8411). + return { + visible: true, + label: 'Create Pull Request', + 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 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 cc1f4965dc8..971d073d557 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,11 +1,26 @@ import { describe, expect, it } from 'vitest' +import type { HostedReviewCreationEligibility } from '../../../src/shared/hosted-review' import { acceptsMobileHostedReviewEligibilityLoad, buildMobileHostedReviewEligibilityLoadKey, eligibilityStateAfterMobileHostedReviewError, + renderedMobileHostedReviewEligibilityState, shouldFetchMobileHostedReviewEligibility } from './use-mobile-hosted-review-eligibility' +function eligibility(): HostedReviewCreationEligibility { + return { + provider: 'github', + review: null, + canCreate: true, + blockedReason: null, + nextAction: null, + defaultBaseRef: 'main', + title: 'feature', + body: '' + } +} + describe('mobile hosted review eligibility loader core', () => { it('does not fetch while disconnected or detached', () => { expect( @@ -56,3 +71,49 @@ describe('mobile hosted review eligibility loader core', () => { 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 and cold loading as an in-flight load seeded from memory', () => { + const remembered = eligibility() + + for (const state of [ + { kind: 'idle' } as const, + { kind: 'loading', eligibility: null } as const + ]) { + expect( + renderedMobileHostedReviewEligibilityState({ state, shouldFetch: true, remembered: null }) + ).toEqual({ kind: 'loading', eligibility: null }) + expect( + renderedMobileHostedReviewEligibilityState({ state, shouldFetch: true, remembered }) + ).toEqual({ kind: 'loading', eligibility: remembered }) + } + }) + + it('passes resolved and refetch states through untouched', () => { + 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({ + state, + shouldFetch: true, + remembered: eligibility() + }) + ).toBe(state) + } + }) + + it('renders idle when a fetch is not possible, hiding stale snapshots in the same render', () => { + expect( + renderedMobileHostedReviewEligibilityState({ + state: { kind: 'ready', eligibility: eligibility() }, + shouldFetch: false, + remembered: eligibility() + }) + ).toEqual({ kind: 'idle' }) + }) +}) 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 ff5a98c433b..1ebea493755 100644 --- a/mobile/src/source-control/use-mobile-hosted-review-eligibility.ts +++ b/mobile/src/source-control/use-mobile-hosted-review-eligibility.ts @@ -6,6 +6,10 @@ import { fetchMobileHostedReviewEligibility, type MobileHostedReviewEligibilityInput } from './mobile-hosted-review-service' +import { + recallHostedReviewEligibility, + rememberHostedReviewEligibility +} from './hosted-review-eligibility-memory' import type { MobileCreatePrEligibilityState } from './mobile-create-pr-action' export type MobileHostedReviewEligibilityLoaderInput = { @@ -60,6 +64,25 @@ export function eligibilityStateAfterMobileHostedReviewError(): MobileCreatePrEl return { kind: 'error' } } +export function renderedMobileHostedReviewEligibilityState(args: { + state: MobileCreatePrEligibilityState + shouldFetch: boolean + remembered: HostedReviewCreationEligibility | null +}): MobileCreatePrEligibilityState { + if (!args.shouldFetch) { + return { kind: 'idle' } + } + const { state, remembered } = args + // Why: under shouldFetch, `idle` only means the fetch effect hasn't run yet, + // and cold `loading` carries no snapshot. Both must render as an in-flight + // load seeded with the last resolved answer so the Create PR row's footprint + // is right on the first painted frame instead of shifting later (#8411). + if (state.kind === 'idle' || (state.kind === 'loading' && !state.eligibility)) { + return { kind: 'loading', eligibility: remembered } + } + return state +} + export function useMobileHostedReviewEligibility( input: MobileHostedReviewEligibilityLoaderInput ): MobileCreatePrEligibilityState { @@ -127,6 +150,11 @@ export function useMobileHostedReviewEligibility( } void fetchMobileHostedReviewEligibility(client, worktreeId, requestInput) .then((eligibility: HostedReviewCreationEligibility | null) => { + if (eligibility) { + // Remember even when superseded: the answer is still valid for the + // identity it was fetched for, and seeds that branch's next open. + rememberHostedReviewEligibility(key.identity, eligibility) + } if (!isCurrent()) { return } @@ -159,5 +187,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({ + state, + shouldFetch, + remembered: recallHostedReviewEligibility(key.identity) + }) }