mirror of
https://github.com/stablyai/orca.git
synced 2026-10-19 16:03:26 +00:00
fix(mobile): keep source-control layout steady while Create PR eligibility loads
The Create PR entry unmounted until the first hostedReview.getCreationEligibility answer arrived, so on a cold open the changed-files list painted first and then shifted down 54pt (createPrBlock marginTop 12 + createPrButton minHeight 42) when the button appeared — while the user was already tapping (#8411). - buildMobileCreatePrAction: cold loading now reserves the row with a disabled placeholder instead of unmounting it. - useMobileHostedReviewEligibility: a fetch-imminent idle frame renders as an in-flight load, so the reservation is present on the first painted frame. - New per-worktree+branch memory of the last resolved eligibility seeds cold loads, so branches whose answer is hidden (existing review, unsupported provider) do not get a placeholder that collapses on every reopen. Fixes #8411
This commit is contained in:
@@ -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> = {}
|
||||
): 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()
|
||||
})
|
||||
})
|
||||
@@ -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<string, HostedReviewCreationEligibility>()
|
||||
|
||||
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()
|
||||
}
|
||||
@@ -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<HostedReviewCreationEligibility> = {}
|
||||
@@ -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)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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' })
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user