From a321ac3c8f4da96f5e76aa015e96baea37b99d4b Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 10 Aug 2026 20:41:31 -0700 Subject: [PATCH] refactor(mobile): use shared source-control decisions (#13453) --- ...-source-control-primary-action-decision.ts | 268 ------------------ ...bile-source-control-primary-action.test.ts | 115 -------- .../mobile-source-control-primary-action.ts | 23 +- ...e-control-primary-action-decision-types.ts | 15 + ...ce-control-primary-action-decision.test.ts | 35 ++- .../source-control-primary-action-decision.ts | 6 +- 6 files changed, 56 insertions(+), 406 deletions(-) delete mode 100644 mobile/src/source-control/mobile-source-control-primary-action-decision.ts diff --git a/mobile/src/source-control/mobile-source-control-primary-action-decision.ts b/mobile/src/source-control/mobile-source-control-primary-action-decision.ts deleted file mode 100644 index eb0927bcff6..00000000000 --- a/mobile/src/source-control/mobile-source-control-primary-action-decision.ts +++ /dev/null @@ -1,268 +0,0 @@ -import type { MobileGitUpstreamStatus } from './mobile-git-status' - -export type MobileSourceControlPrimaryActionKind = - | 'commit' - | 'stage' - | 'push' - | 'pull' - | 'sync' - | 'publish' - -export type MobileSourceControlRemoteOpKind = - | 'push' - | 'force_push' - | 'pull' - | 'sync' - | 'fetch' - | 'fast_forward' - | 'publish' - | 'rebase' - -export type MobileSourceControlPrimaryActionTitleIntent = - | 'commit_in_progress' - | 'force_push_in_progress' - | 'action_in_progress' - | 'remote_operation_in_progress' - | 'remote_operation_blocks_commit' - | 'resolve_conflicts_before_commit' - | 'commit_staged_changes' - | 'enter_commit_message' - | 'stage_all_changes' - | 'stage_file_to_commit' - | 'checkout_branch_before_publish' - | 'publish_branch' - | 'force_push_with_lease' - | 'sync_counts' - | 'pull_count' - | 'push_count' - | 'nothing_to_commit_up_to_date' - -export type MobileSourceControlPrimaryActionDecision = { - kind: MobileSourceControlPrimaryActionKind - disabled: boolean - labelIntent: MobileSourceControlPrimaryActionKind | 'force_push' - titleIntent: MobileSourceControlPrimaryActionTitleIntent - count?: number - ahead?: number - behind?: number - upstreamName?: string - requiresForceWithLease?: boolean -} - -export type MobileSourceControlPrimaryActionDecisionInputs = { - stagedCount: number - hasUnstagedChanges: boolean - hasStageableChanges: boolean - hasPartiallyStagedChanges: boolean - hasMessage: boolean - hasUnresolvedConflicts: boolean - isCommitting: boolean - isRemoteOperationActive: boolean - upstreamStatus: MobileGitUpstreamStatus | undefined - inFlightRemoteOpKind?: MobileSourceControlRemoteOpKind | null - branchCommitsAhead?: number - hasCurrentBranch?: boolean -} - -// Why: Metro cannot load runtime modules from the desktop/root `src/shared` -// tree. Keep this mobile mirror narrow and parity-tested against the shared -// commit-area decision core so the semantic ladder cannot drift silently. -export function resolveMobileSourceControlCommitAreaPrimaryActionDecision( - inputs: MobileSourceControlPrimaryActionDecisionInputs -): MobileSourceControlPrimaryActionDecision { - const { - stagedCount, - hasUnstagedChanges, - hasStageableChanges, - hasMessage, - hasUnresolvedConflicts, - isCommitting, - isRemoteOperationActive, - upstreamStatus, - branchCommitsAhead, - hasCurrentBranch = true - } = inputs - - if (isCommitting) { - return { - kind: 'commit', - labelIntent: 'commit', - titleIntent: 'commit_in_progress', - disabled: true - } - } - - if (isRemoteOperationActive) { - return resolveMobilePrimaryActionDuringRemoteOp(inputs) - } - - if (hasUnresolvedConflicts) { - return { - kind: 'commit', - labelIntent: 'commit', - titleIntent: 'resolve_conflicts_before_commit', - disabled: true - } - } - - const hasStaged = stagedCount > 0 - - if (hasStaged && hasMessage) { - return { - kind: 'commit', - labelIntent: 'commit', - titleIntent: 'commit_staged_changes', - disabled: false - } - } - - if (hasStaged && !hasMessage) { - return { - kind: 'commit', - labelIntent: 'commit', - titleIntent: 'enter_commit_message', - disabled: true - } - } - - if (!hasStaged && hasStageableChanges) { - return { - kind: 'stage', - labelIntent: 'stage', - titleIntent: 'stage_all_changes', - disabled: false - } - } - - if (!upstreamStatus) { - return { - kind: 'commit', - labelIntent: 'commit', - titleIntent: 'stage_file_to_commit', - disabled: true - } - } - - if (!upstreamStatus.hasUpstream) { - if (!hasCurrentBranch) { - return { - kind: 'commit', - labelIntent: 'commit', - titleIntent: 'checkout_branch_before_publish', - disabled: true - } - } - return { - kind: 'publish', - labelIntent: 'publish', - titleIntent: 'publish_branch', - disabled: false - } - } - - if (upstreamStatus.ahead > 0 && upstreamStatus.behind > 0) { - if (shouldForcePushWithLeaseForMobileUpstream(upstreamStatus)) { - return { - kind: 'push', - labelIntent: 'force_push', - titleIntent: 'force_push_with_lease', - disabled: false, - count: branchCommitsAhead, - upstreamName: upstreamStatus.upstreamName, - requiresForceWithLease: true - } - } - return { - kind: 'sync', - labelIntent: 'sync', - titleIntent: 'sync_counts', - disabled: false, - ahead: upstreamStatus.ahead, - behind: upstreamStatus.behind - } - } - - if (upstreamStatus.behind > 0) { - return { - kind: 'pull', - labelIntent: 'pull', - titleIntent: 'pull_count', - disabled: false, - count: upstreamStatus.behind - } - } - - if (upstreamStatus.ahead > 0) { - return { - kind: 'push', - labelIntent: 'push', - titleIntent: 'push_count', - disabled: false, - count: upstreamStatus.ahead - } - } - - return { - kind: 'commit', - labelIntent: 'commit', - titleIntent: hasUnstagedChanges ? 'stage_file_to_commit' : 'nothing_to_commit_up_to_date', - disabled: true - } -} - -function resolveMobilePrimaryActionDuringRemoteOp( - inputs: MobileSourceControlPrimaryActionDecisionInputs -): MobileSourceControlPrimaryActionDecision { - const { inFlightRemoteOpKind, hasUnresolvedConflicts } = inputs - const candidate = resolveMobileSourceControlCommitAreaPrimaryActionDecision({ - ...inputs, - isRemoteOperationActive: false - }) - const inFlightIsPrimaryKind = - inFlightRemoteOpKind === 'push' || - inFlightRemoteOpKind === 'pull' || - inFlightRemoteOpKind === 'sync' || - inFlightRemoteOpKind === 'publish' - - if (inFlightRemoteOpKind === 'force_push') { - return { - kind: 'push', - labelIntent: 'force_push', - titleIntent: 'force_push_in_progress', - disabled: true, - requiresForceWithLease: true - } - } - - if (inFlightIsPrimaryKind && candidate.kind !== inFlightRemoteOpKind) { - return { - kind: inFlightRemoteOpKind, - labelIntent: inFlightRemoteOpKind, - titleIntent: 'action_in_progress', - disabled: true - } - } - - const titleIntent = hasUnresolvedConflicts - ? 'resolve_conflicts_before_commit' - : candidate.kind === 'commit' - ? 'remote_operation_blocks_commit' - : 'remote_operation_in_progress' - - return { - ...candidate, - titleIntent, - disabled: true - } -} - -function shouldForcePushWithLeaseForMobileUpstream( - status: MobileGitUpstreamStatus | undefined -): boolean { - return ( - status?.hasUpstream === true && - status.ahead > 0 && - status.behind > 0 && - status.behindCommitsArePatchEquivalent === true - ) -} diff --git a/mobile/src/source-control/mobile-source-control-primary-action.test.ts b/mobile/src/source-control/mobile-source-control-primary-action.test.ts index 0b127d9bde3..8314ecb2d80 100644 --- a/mobile/src/source-control/mobile-source-control-primary-action.test.ts +++ b/mobile/src/source-control/mobile-source-control-primary-action.test.ts @@ -1,9 +1,4 @@ import { describe, expect, it, vi } from 'vitest' -import { resolveSourceControlCommitAreaPrimaryActionDecision } from '../../../src/shared/source-control-primary-action-decision' -import { - resolveMobileSourceControlCommitAreaPrimaryActionDecision, - type MobileSourceControlPrimaryActionDecisionInputs -} from './mobile-source-control-primary-action-decision' import { buildMobileSourceControlPrimaryAction, type MobileSourceControlPrimaryActionArgs, @@ -159,113 +154,3 @@ describe('buildMobileSourceControlPrimaryAction', () => { expect(action.disabled).toBe(false) }) }) - -function decisionInputs( - overrides: Partial = {} -): MobileSourceControlPrimaryActionDecisionInputs { - return { - stagedCount: 0, - hasUnstagedChanges: false, - hasStageableChanges: false, - hasPartiallyStagedChanges: false, - hasMessage: false, - hasUnresolvedConflicts: false, - isCommitting: false, - isRemoteOperationActive: false, - upstreamStatus: undefined, - ...overrides - } -} - -describe('mobile source-control primary action decision parity', () => { - it.each([ - { - name: 'dirty tree stages first', - input: decisionInputs({ - hasUnstagedChanges: true, - hasStageableChanges: true, - upstreamStatus: { hasUpstream: true, ahead: 0, behind: 2 } - }) - }, - { - name: 'staged message commits', - input: decisionInputs({ stagedCount: 1, hasMessage: true }) - }, - { - name: 'staged without message blocks commit', - input: decisionInputs({ stagedCount: 1, hasMessage: false }) - }, - { - name: 'unresolved conflicts block commit', - input: decisionInputs({ stagedCount: 1, hasMessage: true, hasUnresolvedConflicts: true }) - }, - { - name: 'unpublished branch publishes', - input: decisionInputs({ - upstreamStatus: { hasUpstream: false, ahead: 0, behind: 0 }, - hasCurrentBranch: true - }) - }, - { - name: 'detached head blocks publish', - input: decisionInputs({ - upstreamStatus: { hasUpstream: false, ahead: 0, behind: 0 }, - hasCurrentBranch: false - }) - }, - { - name: 'tracked ahead pushes', - input: decisionInputs({ upstreamStatus: { hasUpstream: true, ahead: 2, behind: 0 } }) - }, - { - name: 'tracked behind pulls', - input: decisionInputs({ upstreamStatus: { hasUpstream: true, ahead: 0, behind: 3 } }) - }, - { - name: 'tracked diverged syncs', - input: decisionInputs({ upstreamStatus: { hasUpstream: true, ahead: 2, behind: 3 } }) - }, - { - name: 'patch-equivalent diverged force-pushes with lease', - input: decisionInputs({ - branchCommitsAhead: 4, - upstreamStatus: { - hasUpstream: true, - upstreamName: 'origin/feature', - ahead: 10, - behind: 2, - behindCommitsArePatchEquivalent: true - } - }) - }, - { - name: 'in-flight pull mirrors pull', - input: decisionInputs({ - isRemoteOperationActive: true, - inFlightRemoteOpKind: 'pull', - upstreamStatus: { hasUpstream: true, ahead: 2, behind: 0 } - }) - }, - { - name: 'in-flight force push mirrors force push', - input: decisionInputs({ - isRemoteOperationActive: true, - inFlightRemoteOpKind: 'force_push', - upstreamStatus: { hasUpstream: true, ahead: 2, behind: 0 } - }) - }, - { - name: 'in-flight push blocks committable candidate', - input: decisionInputs({ - isRemoteOperationActive: true, - inFlightRemoteOpKind: 'push', - stagedCount: 1, - hasMessage: true - }) - } - ])('matches the shared commit-area decision for $name', ({ input }) => { - expect(resolveMobileSourceControlCommitAreaPrimaryActionDecision(input)).toEqual( - resolveSourceControlCommitAreaPrimaryActionDecision(input) - ) - }) -}) diff --git a/mobile/src/source-control/mobile-source-control-primary-action.ts b/mobile/src/source-control/mobile-source-control-primary-action.ts index f62a7770d16..2a993967b52 100644 --- a/mobile/src/source-control/mobile-source-control-primary-action.ts +++ b/mobile/src/source-control/mobile-source-control-primary-action.ts @@ -1,14 +1,17 @@ import { - resolveMobileSourceControlCommitAreaPrimaryActionDecision, - type MobileSourceControlPrimaryActionDecision, - type MobileSourceControlPrimaryActionKind, - type MobileSourceControlRemoteOpKind -} from './mobile-source-control-primary-action-decision' + resolveSourceControlCommitAreaPrimaryActionDecision, + type SourceControlCommitAreaPrimaryActionDecision, + type SourceControlRemoteOpKind +} from '../../../src/shared/source-control-primary-action-decision' import type { MobileGitBranchCompareResult } from './mobile-branch-compare' import type { MobileGitStatusResult } from './mobile-git-status' type GitStep = { method: string; params?: Record } +type MobileSourceControlPrimaryActionKind = SourceControlCommitAreaPrimaryActionDecision['kind'] +type MobileSourceControlPrimaryActionDecision = SourceControlCommitAreaPrimaryActionDecision +type MobileSourceControlRemoteOpKind = SourceControlRemoteOpKind + export type MobileSourceControlPrimaryAction = { kind: MobileSourceControlPrimaryActionKind label: string @@ -44,7 +47,7 @@ export type MobileSourceControlPrimaryActionArgs = { export function buildMobileSourceControlPrimaryAction( args: MobileSourceControlPrimaryActionArgs ): MobileSourceControlPrimaryAction { - const decision = resolveMobileSourceControlCommitAreaPrimaryActionDecision({ + const decision = resolveSourceControlCommitAreaPrimaryActionDecision({ stagedCount: args.stagedCount, hasUnstagedChanges: args.unstagedCount > 0, hasStageableChanges: args.stageablePaths.length > 0, @@ -167,8 +170,16 @@ function getMobilePrimaryActionHint(decision: MobileSourceControlPrimaryActionDe return 'Stage at least one file to commit.' case 'checkout_branch_before_publish': return 'Check out a branch before publishing commits.' + case 'checking_review_status': + return 'Checking review status.' + case 'review_already_merged': + return 'Nothing to commit. The review is already merged.' case 'publish_branch': return 'Publish this branch to origin.' + case 'push_linked_review': + return 'Push updates to the linked review branch.' + case 'linked_review_target_unavailable': + return 'The linked review branch is unavailable.' case 'force_push_with_lease': return 'Force push with lease to update the remote branch.' case 'sync_counts': diff --git a/src/shared/source-control-primary-action-decision-types.ts b/src/shared/source-control-primary-action-decision-types.ts index 1769dbe0958..c1e2147b315 100644 --- a/src/shared/source-control-primary-action-decision-types.ts +++ b/src/shared/source-control-primary-action-decision-types.ts @@ -60,6 +60,21 @@ export type SourceControlPrimaryActionDecision = { requiresForceWithLease?: boolean } +export type SourceControlCommitAreaPrimaryActionDecision = Omit< + SourceControlPrimaryActionDecision, + 'kind' | 'labelIntent' | 'titleIntent' +> & { + kind: Exclude + labelIntent: Exclude< + SourceControlPrimaryActionDecision['labelIntent'], + 'create_pr_intent' | 'create_pr' + > + titleIntent: Exclude< + SourceControlPrimaryActionTitleIntent, + 'prepare_review' | 'create_review' | 'checking_review_creation' + > +} + export type SourceControlPrimaryActionDecisionInputs = { stagedCount: number hasUnstagedChanges: boolean diff --git a/src/shared/source-control-primary-action-decision.test.ts b/src/shared/source-control-primary-action-decision.test.ts index 6f153d74bbe..892d27cbffb 100644 --- a/src/shared/source-control-primary-action-decision.test.ts +++ b/src/shared/source-control-primary-action-decision.test.ts @@ -159,25 +159,30 @@ describe('source-control primary action decision', () => { expect(resolveSourceControlCommitAreaPrimaryActionDecision(input).kind).toBe('commit') }) + it('keeps in-flight review intents out of commit-area decisions', () => { + const input = inputs({ isPrIntentInFlight: true }) + expect(resolveSourceControlPrimaryActionDecision(input).kind).toBe('create_pr_intent') + expect(resolveSourceControlCommitAreaPrimaryActionDecision(input).kind).toBe('commit') + }) + it('returns disabled create review while hosted-review creation eligibility is loading', () => { - const result = resolveSourceControlPrimaryActionDecision( - inputs({ - upstreamStatus: { hasUpstream: true, ahead: 0, behind: 0 }, - hostedReviewCreation: { - provider: 'gitlab', - review: null, - canCreate: false, - blockedReason: null, - nextAction: null, - reviewLookupOutcome: 'not_found' - }, - isHostedReviewCreationLoading: true - }) - ) - expect(result).toMatchObject({ + const input = inputs({ + upstreamStatus: { hasUpstream: true, ahead: 0, behind: 0 }, + hostedReviewCreation: { + provider: 'gitlab', + review: null, + canCreate: false, + blockedReason: null, + nextAction: null, + reviewLookupOutcome: 'not_found' + }, + isHostedReviewCreationLoading: true + }) + expect(resolveSourceControlPrimaryActionDecision(input)).toMatchObject({ kind: 'create_pr', titleIntent: 'checking_review_creation', disabled: true }) + expect(resolveSourceControlCommitAreaPrimaryActionDecision(input).kind).toBe('commit') }) }) diff --git a/src/shared/source-control-primary-action-decision.ts b/src/shared/source-control-primary-action-decision.ts index acdaf27cbf8..3a06c6f9133 100644 --- a/src/shared/source-control-primary-action-decision.ts +++ b/src/shared/source-control-primary-action-decision.ts @@ -3,6 +3,7 @@ import { supportsHostedReviewCreation } from './hosted-review-creation-providers import { resolveCreateReviewIntentEligibility } from './source-control-create-review-intent' import { resolveSourceControlPrimaryActionDuringRemoteOp } from './source-control-primary-action-in-flight' import type { + SourceControlCommitAreaPrimaryActionDecision, SourceControlPrimaryActionDecision, SourceControlPrimaryActionDecisionInputs } from './source-control-primary-action-decision-types' @@ -11,6 +12,7 @@ import { resolveUnpublishedSourceControlPrimaryAction } from './source-control-p export type { SourceControlPrimaryActionKind, SourceControlRemoteOpKind, + SourceControlCommitAreaPrimaryActionDecision, SourceControlPrimaryActionDecision, SourceControlPrimaryActionDecisionInputs } from './source-control-primary-action-decision-types' @@ -221,14 +223,14 @@ function shouldOfferCreateReviewLoadingAction( export function resolveSourceControlCommitAreaPrimaryActionDecision( inputs: SourceControlPrimaryActionDecisionInputs -): SourceControlPrimaryActionDecision { +): SourceControlCommitAreaPrimaryActionDecision { // Why: review creation is additive chrome. Commit/mobile bottom areas keep // the local/remote action they would have without review eligibility. return resolveSourceControlPrimaryActionDecision({ ...inputs, hostedReviewCreation: null, isPrIntentInFlight: false - }) + }) as SourceControlCommitAreaPrimaryActionDecision } function resolveCreatePrIntentDecision(