diff --git a/mobile/src/components/MobilePrComposeSheet.tsx b/mobile/src/components/MobilePrComposeSheet.tsx index cf25c8aab..ac0e90797 100644 --- a/mobile/src/components/MobilePrComposeSheet.tsx +++ b/mobile/src/components/MobilePrComposeSheet.tsx @@ -11,7 +11,7 @@ type Props = { // Head branch — enables the base≠head guard and the "from " hint. head?: string | null onClose: () => void - onCreated: (url: string) => void + onCreated: (url: string, warning?: string) => void } // BottomDrawer wrapper around the inline compose form, for full-screen roots diff --git a/mobile/src/components/pr-sidebar/MobilePrComposeForm.tsx b/mobile/src/components/pr-sidebar/MobilePrComposeForm.tsx index 9c40c0312..765a30dba 100644 --- a/mobile/src/components/pr-sidebar/MobilePrComposeForm.tsx +++ b/mobile/src/components/pr-sidebar/MobilePrComposeForm.tsx @@ -36,7 +36,7 @@ type Props = { // Head branch — enables the base≠head guard and the "from " hint. head?: string | null onCancel: () => void - onCreated: (url: string) => void + onCreated: (url: string, warning?: string) => void } // PR compose form body: title/body/base/draft with AI prefill (git.generate @@ -132,7 +132,13 @@ export function MobilePrComposeForm({ }) if (outcome.ok) { triggerSuccess() - onCreated(outcome.url) + const warning = outcome.linkError + ? `${copy.titleLabel} created, but Orca could not refresh it yet.` + : undefined + if (warning) { + setError(warning) + } + onCreated(outcome.url, warning) } else { triggerError() setError(outcome.error) @@ -145,6 +151,7 @@ export function MobilePrComposeForm({ body, canSubmit, client, + copy.titleLabel, draft, head, onCreated, diff --git a/mobile/src/components/pr-sidebar/PrSidebarCreateEmptyState.tsx b/mobile/src/components/pr-sidebar/PrSidebarCreateEmptyState.tsx index 079f0f92b..a11bdec59 100644 --- a/mobile/src/components/pr-sidebar/PrSidebarCreateEmptyState.tsx +++ b/mobile/src/components/pr-sidebar/PrSidebarCreateEmptyState.tsx @@ -25,6 +25,7 @@ export function PrSidebarCreateEmptyState({ client, worktreeId, gitBranch, onCre const [prefill, setPrefill] = useState(null) const [mode, setMode] = useState('choose') const [loading, setLoading] = useState(false) + const [createWarning, setCreateWarning] = useState(null) // A persisted linkedPR while the branch shows no PR means the linked PR could // not be resolved. Mention it, but keep link editing out of this desktop-parity // create surface. @@ -56,6 +57,7 @@ export function PrSidebarCreateEmptyState({ client, worktreeId, gitBranch, onCre if (!client || loading) { return } + setCreateWarning(null) setLoading(true) try { // Git-status fields are best-effort here (the sidebar has no working-tree @@ -90,8 +92,9 @@ export function PrSidebarCreateEmptyState({ client, worktreeId, gitBranch, onCre prefill={prefill} head={gitBranch} onCancel={() => setMode('choose')} - onCreated={(url) => { + onCreated={(url, warning) => { setMode('choose') + setCreateWarning(warning ?? null) openMobilePrUrl(url) onCreated() }} @@ -144,6 +147,7 @@ export function PrSidebarCreateEmptyState({ client, worktreeId, gitBranch, onCre ? `${gitBranch} is not linked to an open PR.` : 'The current branch is not linked to an open PR.'} + {createWarning ? {createWarning} : null} ) diff --git a/mobile/src/source-control/MobileSourceControlModals.tsx b/mobile/src/source-control/MobileSourceControlModals.tsx index 637d0b6e4..bd47a1901 100644 --- a/mobile/src/source-control/MobileSourceControlModals.tsx +++ b/mobile/src/source-control/MobileSourceControlModals.tsx @@ -28,6 +28,8 @@ export function MobileSourceControlModals({ state, worktreeId, actionSheetAction localBranches, createdPrUrl, setCreatedPrUrl, + createdPrWarning, + setCreatedPrWarning, status, branchLabel, loadStatus, @@ -79,9 +81,10 @@ export function MobileSourceControlModals({ state, worktreeId, actionSheetAction prefill={prPrefill ?? { provider: 'github', base: 'main', title: branchLabel, body: '' }} head={status?.branch ?? null} onClose={() => setShowPrSheet(false)} - onCreated={(url) => { + onCreated={(url, warning) => { setShowPrSheet(false) setCreatedPrUrl(url) + setCreatedPrWarning(warning ?? null) void loadStatus({ preserveReadyOnFailure: true, force: true }) }} /> @@ -108,15 +111,23 @@ export function MobileSourceControlModals({ state, worktreeId, actionSheetAction { if (createdPrUrl) { openMobilePrUrl(createdPrUrl) } setCreatedPrUrl(null) + setCreatedPrWarning(null) + }} + onCancel={() => { + setCreatedPrUrl(null) + setCreatedPrWarning(null) }} - onCancel={() => setCreatedPrUrl(null)} /> ) diff --git a/mobile/src/source-control/mobile-branch-base-ref.test.ts b/mobile/src/source-control/mobile-branch-base-ref.test.ts new file mode 100644 index 000000000..5720637a3 --- /dev/null +++ b/mobile/src/source-control/mobile-branch-base-ref.test.ts @@ -0,0 +1,70 @@ +import { describe, expect, it } from 'vitest' +import { resolveMobileBranchCompareBaseRef } from './mobile-branch-base-ref' +import type { RpcClient } from '../transport/rpc-client' + +type RpcResponse = + | { ok: true; result: unknown } + | { ok: false; error: { code: string; message: string } } + +function ok(result: unknown): RpcResponse { + return { ok: true, result } +} + +function fail(message: string): RpcResponse { + return { ok: false, error: { code: 'runtime_error', message } } +} + +function clientWith(responses: RpcResponse[]) { + const calls: { method: string; params?: Record }[] = [] + return { + calls, + client: { + sendRequest: async (method: string, params?: Record) => { + calls.push({ method, params }) + return responses.shift() ?? fail(`unexpected ${method}`) + } + } as Pick as RpcClient + } +} + +describe('resolveMobileBranchCompareBaseRef', () => { + it('prefers the per-worktree base ref over the repo default', async () => { + const { client, calls } = clientWith([ + ok({ worktree: { baseRef: 'origin/release' } }), + ok({ repos: [{ id: 'repo-1', worktreeBaseRef: 'origin/main' }] }) + ]) + + await expect(resolveMobileBranchCompareBaseRef(client, 'repo-1::/tmp/wt')).resolves.toBe( + 'origin/release' + ) + expect(calls.map((call) => call.method)).toEqual(['worktree.show', 'repo.list']) + }) + + it('falls back to repo worktreeBaseRef when the worktree has no pinned base', async () => { + const { client } = clientWith([ + ok({ worktree: { baseRef: null } }), + ok({ repos: [{ id: 'repo-1', worktreeBaseRef: 'origin/main' }] }) + ]) + + await expect(resolveMobileBranchCompareBaseRef(client, 'repo-1::/tmp/wt')).resolves.toBe( + 'origin/main' + ) + }) + + it('falls back to repo.baseRefDefault when metadata reads have no base', async () => { + const { client, calls } = clientWith([ + fail('old host'), + ok({ repos: [{ id: 'repo-1', worktreeBaseRef: null }] }), + ok({ defaultBaseRef: 'origin/main' }) + ]) + + await expect(resolveMobileBranchCompareBaseRef(client, 'repo-1::/tmp/wt')).resolves.toBe( + 'origin/main' + ) + expect(calls.map((call) => call.method)).toEqual([ + 'worktree.show', + 'repo.list', + 'repo.baseRefDefault' + ]) + }) +}) diff --git a/mobile/src/source-control/mobile-branch-base-ref.ts b/mobile/src/source-control/mobile-branch-base-ref.ts index fae287ae3..dbe33a43d 100644 --- a/mobile/src/source-control/mobile-branch-base-ref.ts +++ b/mobile/src/source-control/mobile-branch-base-ref.ts @@ -6,6 +6,10 @@ type RuntimeRepoSummary = { worktreeBaseRef?: string | null } +type RuntimeWorktreeSummary = { + baseRef?: string | null +} + function isRecord(value: unknown): value is Record { return typeof value === 'object' && value !== null } @@ -40,6 +44,15 @@ function readDefaultBaseRef(value: unknown): string | null { return typeof value.defaultBaseRef === 'string' ? value.defaultBaseRef.trim() || null : null } +function readWorktreeSummary(value: unknown): RuntimeWorktreeSummary | null { + if (!isRecord(value) || !isRecord(value.worktree)) { + return null + } + return { + baseRef: typeof value.worktree.baseRef === 'string' ? value.worktree.baseRef : null + } +} + export async function resolveMobileBranchCompareBaseRef( client: RpcClient, worktreeId: string @@ -49,15 +62,23 @@ export async function resolveMobileBranchCompareBaseRef( return null } - let repoBaseRef: string | null = null - const repoResponse = await client.sendRequest('repo.list') - if (repoResponse.ok) { - const repo = readRepoSummaries(repoResponse.result).find((candidate) => candidate.id === repoId) - repoBaseRef = repo?.worktreeBaseRef?.trim() || null + const [worktreeResponse, repoResponse] = await Promise.all([ + client.sendRequest('worktree.show', { worktree: `id:${worktreeId}` }).catch(() => null), + client.sendRequest('repo.list').catch(() => null) + ]) + if (worktreeResponse?.ok) { + const worktreeBaseRef = readWorktreeSummary(worktreeResponse.result)?.baseRef?.trim() || null + if (worktreeBaseRef) { + return worktreeBaseRef + } } - if (repoBaseRef) { - return repoBaseRef + if (repoResponse?.ok) { + const repo = readRepoSummaries(repoResponse.result).find((candidate) => candidate.id === repoId) + const repoBaseRef = repo?.worktreeBaseRef?.trim() || null + if (repoBaseRef) { + return repoBaseRef + } } const defaultResponse = await client.sendRequest('repo.baseRefDefault', { repo: `id:${repoId}` }) diff --git a/mobile/src/source-control/mobile-pr-create.test.ts b/mobile/src/source-control/mobile-pr-create.test.ts index cbea4a087..e82fb4eed 100644 --- a/mobile/src/source-control/mobile-pr-create.test.ts +++ b/mobile/src/source-control/mobile-pr-create.test.ts @@ -69,7 +69,10 @@ describe('buildMobilePrCreateParams', () => { describe('createMobilePr', () => { it('returns the url on success', async () => { - const client = clientWith([ok({ ok: true, number: 42, url: 'https://github.com/o/r/pull/42' })]) + const client = clientWith([ + ok({ ok: true, number: 42, url: 'https://github.com/o/r/pull/42' }), + ok({ worktree: { linkedPR: 42 } }) + ]) await expect( createMobilePr(client, 'repo-1::/tmp/wt', { provider: 'github', @@ -80,6 +83,56 @@ describe('createMobilePr', () => { }) ).resolves.toEqual({ ok: true, number: 42, url: 'https://github.com/o/r/pull/42' }) expect(client.calls[0].method).toBe('hostedReview.create') + expect(client.calls[1]).toEqual({ + method: 'worktree.set', + params: { worktree: 'id:repo-1::/tmp/wt', baseRef: 'main', linkedPR: 42 } + }) + }) + + it('links created merge requests through the provider-specific worktree field', async () => { + const client = clientWith([ + ok({ ok: true, number: 7, url: 'https://gitlab.com/o/r/-/merge_requests/7' }), + ok({ worktree: { linkedGitLabMR: 7 } }) + ]) + await expect( + createMobilePr(client, 'repo-1::/tmp/wt', { + provider: 'gitlab', + base: 'main', + title: 'T', + body: '', + draft: false + }) + ).resolves.toEqual({ + ok: true, + number: 7, + url: 'https://gitlab.com/o/r/-/merge_requests/7' + }) + expect(client.calls[1]).toEqual({ + method: 'worktree.set', + params: { worktree: 'id:repo-1::/tmp/wt', baseRef: 'main', linkedGitLabMR: 7 } + }) + }) + + it('keeps the created url when the metadata link refresh fails', async () => { + const client = clientWith([ + ok({ ok: true, number: 42, url: 'https://github.com/o/r/pull/42' }), + fail('metadata failed') + ]) + + await expect( + createMobilePr(client, 'repo-1::/tmp/wt', { + provider: 'github', + base: 'main', + title: 'T', + body: '', + draft: false + }) + ).resolves.toEqual({ + ok: true, + number: 42, + url: 'https://github.com/o/r/pull/42', + linkError: 'metadata failed' + }) }) it('maps a host failure result to { ok:false }', async () => { diff --git a/mobile/src/source-control/mobile-pr-create.ts b/mobile/src/source-control/mobile-pr-create.ts index bd2d6aaab..5dba2167d 100644 --- a/mobile/src/source-control/mobile-pr-create.ts +++ b/mobile/src/source-control/mobile-pr-create.ts @@ -5,6 +5,7 @@ import type { } from '../../../src/shared/hosted-review' import type { RpcClient } from '../transport/rpc-client' import type { RpcSuccess } from '../transport/types' +import { linkMobileHostedReview } from './mobile-pr-link' // The mobile worktree id is `${repoId}::${path}`; the repo selector the host // hosted-review RPCs expect is `id:${repoId}`. @@ -130,7 +131,7 @@ export function buildMobilePrCreateParams( } export type MobilePrCreateOutcome = - | { ok: true; url: string; number: number } + | { ok: true; url: string; number: number; linkError?: string } | { ok: false; error: string } export async function createMobilePr( @@ -148,7 +149,23 @@ export async function createMobilePr( } const result = (response as RpcSuccess).result as CreateHostedReviewResult if (result.ok) { - return { ok: true, url: result.url, number: result.number } + const linked = await linkMobileHostedReview( + client, + worktreeId, + input.provider, + result.number, + { + // Why: mobile branch compare cannot infer the new hosted review's target + // base from renderer cache; persist the submitted base for the refresh. + baseRef: input.base + } + ) + return { + ok: true, + url: result.url, + number: result.number, + ...(linked.ok ? {} : { linkError: linked.error }) + } } return { ok: false, error: result.error || 'Failed to create pull request' } } catch (err) { diff --git a/mobile/src/source-control/mobile-pr-link.test.ts b/mobile/src/source-control/mobile-pr-link.test.ts index c04cf9734..5500d884c 100644 --- a/mobile/src/source-control/mobile-pr-link.test.ts +++ b/mobile/src/source-control/mobile-pr-link.test.ts @@ -1,6 +1,11 @@ import { describe, expect, it, vi } from 'vitest' import type { RpcClient } from '../transport/rpc-client' -import { buildWorktreeSetLinkParams, fetchWorktreeLinkedPR, linkMobilePr } from './mobile-pr-link' +import { + buildWorktreeSetHostedReviewLinkParams, + buildWorktreeSetLinkParams, + fetchWorktreeLinkedPR, + linkMobilePr +} from './mobile-pr-link' describe('buildWorktreeSetLinkParams', () => { it('sets linkedPR to a number when linking', () => { @@ -18,6 +23,39 @@ describe('buildWorktreeSetLinkParams', () => { }) }) +describe('buildWorktreeSetHostedReviewLinkParams', () => { + it.each([ + ['github', 'linkedPR'], + ['gitlab', 'linkedGitLabMR'], + ['bitbucket', 'linkedBitbucketPR'], + ['azure-devops', 'linkedAzureDevOpsPR'], + ['gitea', 'linkedGiteaPR'] + ] as const)('maps %s reviews to the matching worktree field', (provider, key) => { + expect(buildWorktreeSetHostedReviewLinkParams('repo42::/p', provider, 12)).toEqual({ + worktree: 'id:repo42::/p', + [key]: 12 + }) + }) + + it('includes a submitted base ref so mobile diff review refreshes against the review base', () => { + expect( + buildWorktreeSetHostedReviewLinkParams('repo42::/p', 'github', 12, { + baseRef: ' origin/release ' + }) + ).toEqual({ + worktree: 'id:repo42::/p', + baseRef: 'origin/release', + linkedPR: 12 + }) + }) + + it('does not invent a metadata field for unsupported providers', () => { + expect(buildWorktreeSetHostedReviewLinkParams('repo42::/p', 'unsupported', 12)).toEqual({ + worktree: 'id:repo42::/p' + }) + }) +}) + describe('fetchWorktreeLinkedPR', () => { const client = (result: unknown, okFlag = true) => ({ diff --git a/mobile/src/source-control/mobile-pr-link.ts b/mobile/src/source-control/mobile-pr-link.ts index 2954f53cd..1befb2eeb 100644 --- a/mobile/src/source-control/mobile-pr-link.ts +++ b/mobile/src/source-control/mobile-pr-link.ts @@ -1,11 +1,11 @@ import type { RpcClient } from '../transport/rpc-client' import type { RpcSuccess } from '../transport/types' +import type { HostedReviewProvider } from '../../../src/shared/hosted-review' -// Link / unlink an existing PR to the current worktree via worktree.set (the same -// path desktop's "Link another PR" uses). GitHub-scoped: it writes the worktree's -// `linkedPR` key, matching desktop where linking is GitHub-only (GitLab/Bitbucket -// use separate linked* keys). linkedPR is tri-state on the host: a number sets the -// link, null clears it. worktree.set is allowlisted for mobile. +// Link / unlink review metadata via worktree.set (the same path desktop uses). +// GitHub's existing manual link flow writes linkedPR; hosted-review creation maps +// each provider to its own linked* field so mobile follow-up reads get the same +// authoritative hint as desktop. export type MobilePrLinkOutcome = { ok: true } | { ok: false; error: string } @@ -17,6 +17,33 @@ export function buildWorktreeSetLinkParams( return { worktree: `id:${worktreeId}`, linkedPR } } +export function buildWorktreeSetHostedReviewLinkParams( + worktreeId: string, + provider: HostedReviewProvider, + number: number | null, + options?: { baseRef?: string | null } +): Record { + const trimmedBaseRef = options?.baseRef?.trim() + const base = { + worktree: `id:${worktreeId}`, + ...(trimmedBaseRef ? { baseRef: trimmedBaseRef } : {}) + } + switch (provider) { + case 'github': + return { ...base, linkedPR: number } + case 'gitlab': + return { ...base, linkedGitLabMR: number } + case 'bitbucket': + return { ...base, linkedBitbucketPR: number } + case 'azure-devops': + return { ...base, linkedAzureDevOpsPR: number } + case 'gitea': + return { ...base, linkedGiteaPR: number } + case 'unsupported': + return base + } +} + async function setLinkedPr( client: Pick, worktreeId: string, @@ -49,6 +76,33 @@ export function linkMobilePr( return setLinkedPr(client, worktreeId, prNumber) } +export async function linkMobileHostedReview( + client: Pick, + worktreeId: string, + provider: HostedReviewProvider, + number: number, + options?: { baseRef?: string | null } +): Promise { + const params = buildWorktreeSetHostedReviewLinkParams(worktreeId, provider, number, options) + if (Object.keys(params).length === 1) { + return { ok: true } + } + try { + const response = await client.sendRequest('worktree.set', params) + if (!response.ok) { + return { ok: false, error: response.error?.message || 'Failed to update linked review' } + } + return { ok: true } + } catch (err) { + // Why: the review was already created; normalize link failures so callers can + // surface a non-fatal refresh problem instead of losing the created URL. + return { + ok: false, + error: err instanceof Error ? err.message : 'Failed to update linked review' + } + } +} + export function unlinkMobilePr( client: Pick, worktreeId: string 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 9828622ee..875715ad9 100644 --- a/mobile/src/source-control/use-mobile-source-control-state.ts +++ b/mobile/src/source-control/use-mobile-source-control-state.ts @@ -53,6 +53,7 @@ export function useMobileSourceControlState(params: MobileSourceControlStatePara const [showBranchPicker, setShowBranchPicker] = useState(false) const [localBranches, setLocalBranches] = useState(null) const [createdPrUrl, setCreatedPrUrl] = useState(null) + const [createdPrWarning, setCreatedPrWarning] = useState(null) const [prPrefill, setPrPrefill] = useState(null) const [discardTarget, setDiscardTarget] = useState(null) const [showActionSheet, setShowActionSheet] = useState(false) @@ -224,6 +225,8 @@ export function useMobileSourceControlState(params: MobileSourceControlStatePara localBranches, createdPrUrl, setCreatedPrUrl, + createdPrWarning, + setCreatedPrWarning, prPrefill, discardTarget, setDiscardTarget,