diff --git a/mobile/src/tasks/github-project-host-routing-source.test.ts b/mobile/src/tasks/github-project-host-routing-source.test.ts index 3c567a819ad..693f741714e 100644 --- a/mobile/src/tasks/github-project-host-routing-source.test.ts +++ b/mobile/src/tasks/github-project-host-routing-source.test.ts @@ -108,14 +108,14 @@ describe('mobile GitHub Project host routing boundary', () => { ) // Per method, not once per file: dropping prRepo from a single mutation must fail here. for (const method of [ - 'fetchResolveReviewThread', - 'fetchAddPRReviewCommentReply', - 'fetchAddIssueComment', - 'fetchRequestPRReviewers', - 'fetchRerunPRChecks', - 'fetchMergePR' + 'github.resolveReviewThread', + 'github.addPRReviewCommentReply', + 'github.addIssueComment', + 'github.requestPRReviewers', + 'github.rerunPRChecks', + 'github.mergePR' ]) { - const offset = projectMutationAdapter.indexOf(`${method}(`) + const offset = projectMutationAdapter.indexOf(`'${method}'`) expect(offset, `${method} must remain wired`).toBeGreaterThan(-1) // Bounded to this adapter method, so a neighbour's prRepo cannot satisfy it. expect( diff --git a/mobile/src/tasks/native-host-task-detail-operations.test.ts b/mobile/src/tasks/native-host-task-detail-operations.test.ts new file mode 100644 index 00000000000..5ac77ea2991 --- /dev/null +++ b/mobile/src/tasks/native-host-task-detail-operations.test.ts @@ -0,0 +1,27 @@ +import { expect, it, vi } from 'vitest' +import type { RpcRequestSender } from '../transport/rpc-client' +import { nativeHostTaskDetailOperations } from './native-host-task-detail-operations' + +it('waits for raw Linear requests so a comments transport rejection wins over an issue refusal', async () => { + let rejectComments!: (error: Error) => void + const comments = new Promise((_resolve, reject) => { + rejectComments = reject + }) + const sendRequest = vi.fn((method) => + method === 'linear.getIssue' + ? Promise.resolve({ + id: 'test', + _meta: { runtimeId: 'host' }, + ok: false, + error: { code: 'refused', message: 'issue refused' } + }) + : comments + ) + const outcome = nativeHostTaskDetailOperations({ sendRequest }) + .loadLinear({ issueId: 'issue-1', workspaceId: 'workspace-1' }) + .catch((error: unknown) => error) + await new Promise((resolve) => setTimeout(resolve, 0)) + const commentsError = new Error('comments transport failed') + rejectComments(commentsError) + await expect(outcome).resolves.toBe(commentsError) +}) diff --git a/mobile/src/tasks/native-host-task-detail-operations.ts b/mobile/src/tasks/native-host-task-detail-operations.ts index 40132a85c13..01cce9f7527 100644 --- a/mobile/src/tasks/native-host-task-detail-operations.ts +++ b/mobile/src/tasks/native-host-task-detail-operations.ts @@ -81,8 +81,13 @@ export function nativeHostTaskDetailOperations(client: RpcRequestSender): HostTa { timeoutMs: 30_000 } ) ]) - const issue = await successfulResult(issueResponse) - const comments = await optionalComments(commentsResponse) + if (!issueResponse.ok) { + throw new Error(issueResponse.error.message) + } + const issue = issueResponse.result as LinearIssue | null + const comments = commentsResponse.ok + ? ((commentsResponse.result as DetailComment[]) ?? []) + : [] if (!issue) { throw new Error('Details not found') } @@ -91,13 +96,6 @@ export function nativeHostTaskDetailOperations(client: RpcRequestSender): HostTa } } -/** Tolerates a refusal envelope only. A transport rejection still fails the detail load, so a - * timed-out comment read cannot render as an issue that simply has no comments. */ -async function optionalComments(request: Promise): Promise { - const response = (await request) as { ok: boolean; result?: unknown } - return response.ok ? ((response.result as DetailComment[]) ?? []) : [] -} - async function successfulResult(request: Promise): Promise { const response = (await request) as { ok: boolean diff --git a/mobile/src/tasks/native-host-task-project-mutation-operations.test.ts b/mobile/src/tasks/native-host-task-project-mutation-operations.test.ts new file mode 100644 index 00000000000..e1898d7142a --- /dev/null +++ b/mobile/src/tasks/native-host-task-project-mutation-operations.test.ts @@ -0,0 +1,113 @@ +import { describe, expect, it, vi } from 'vitest' +import type { RpcRequestSender } from '../transport/rpc-client' +import type { HostTaskProjectMutationOperations } from './host-task-project-mutation-operations' +import { nativeHostTaskProjectMutationOperations } from './native-host-task-project-mutation-operations' + +const target = { owner: 'orca', repo: 'orca', host: 'github.com', number: 7, type: 'pr' as const } +const mutations: Array< + [string, (ops: HostTaskProjectMutationOperations) => Promise, string] +> = [ + [ + 'updateItem', + (ops) => ops.updateItem(target, { title: 'title' }), + 'Failed to update GitHub item' + ], + [ + 'updateIssue', + (ops) => ops.updateItem({ ...target, type: 'issue' }, {}), + 'Failed to update GitHub item' + ], + ['updateComment', (ops) => ops.updateComment(target, 1, 'body'), 'Failed to edit comment'], + ['deleteComment', (ops) => ops.deleteComment(target, 1), 'Failed to delete comment'], + ['updateMetadata', (ops) => ops.updateMetadata(target, {}), 'Failed to update GitHub item'], + [ + 'updateField', + (ops) => + ops.updateField({ ...target, projectId: 'p', itemId: 'i' }, 'f', { + kind: 'text', + text: 'value' + }), + 'Failed to update project field' + ], + [ + 'clearField', + (ops) => ops.updateField({ ...target, projectId: 'p', itemId: 'i' }, 'f', null), + 'Failed to update project field' + ], + ['updateIssueType', (ops) => ops.updateIssueType(target, null), 'Failed to update issue type'], + [ + 'replyReviewComment', + (ops) => ops.replyReviewComment(target, 'repo-1', { commentId: 1, body: 'body' }), + 'Failed to reply' + ], + [ + 'addConversationComment', + (ops) => ops.addConversationComment(target, 'repo-1', 'body'), + 'Failed to reply' + ], + [ + 'requestReviewers', + (ops) => ops.requestReviewers(target, 'repo-1', ['reviewer']), + 'Failed to request reviewers' + ], + [ + 'rerunChecks', + (ops) => ops.rerunChecks(target, 'repo-1', { failedOnly: true }), + 'Failed to rerun checks' + ], + ['merge', (ops) => ops.merge(target, 'repo-1', 'squash'), 'Failed to merge pull request'] +] + +function operations(result: unknown) { + const sendRequest = vi + .fn() + .mockResolvedValue({ id: 'test', _meta: { runtimeId: 'host' }, ok: true, result }) + return nativeHostTaskProjectMutationOperations({ sendRequest }) +} + +describe('Project mutation result acceptance', () => { + it.each(mutations)( + '%s rejects absent results, accepts missing ok, and reports refusal', + async (_name, mutate, fallback) => { + for (const result of [null, undefined]) { + await expect(mutate(operations(result))).rejects.toBeInstanceOf(TypeError) + } + for (const result of [{}, { ok: undefined }, { ok: true }]) { + await expect(mutate(operations(result))).resolves.toBeUndefined() + } + await expect(mutate(operations({ ok: false }))).rejects.toThrow(fallback) + } + ) + + it('addComment requires truthy ok and preserves the host message', async () => { + for (const result of [null, undefined]) { + await expect(operations(result).addComment(target, 'body')).rejects.toBeInstanceOf(TypeError) + } + for (const result of [{}, { ok: undefined }, { ok: false }]) { + await expect(operations(result).addComment(target, 'body')).rejects.toThrow( + 'Failed to add comment' + ) + } + await expect( + operations({ ok: false, error: { message: 'host refusal' } }).addComment(target, 'body') + ).rejects.toThrow('host refusal') + await expect(operations({ ok: true }).addComment(target, 'body')).resolves.toBeUndefined() + }) + + it('merge preserves its string refusal', async () => { + await expect( + operations({ ok: false, error: 'merge refusal' }).merge(target, 'repo-1', 'merge') + ).rejects.toThrow('merge refusal') + }) + + it('resolveReviewThread requires the literal true result', async () => { + for (const result of [null, undefined, {}, { ok: true }, false]) { + await expect( + operations(result).resolveReviewThread(target, 'repo-1', 'thread', true) + ).rejects.toThrow('Failed to resolve thread') + } + await expect( + operations(true).resolveReviewThread(target, 'repo-1', 'thread', true) + ).resolves.toBeUndefined() + }) +}) diff --git a/mobile/src/tasks/native-host-task-project-mutation-operations.ts b/mobile/src/tasks/native-host-task-project-mutation-operations.ts index 875bb30a6f8..f983405291e 100644 --- a/mobile/src/tasks/native-host-task-project-mutation-operations.ts +++ b/mobile/src/tasks/native-host-task-project-mutation-operations.ts @@ -4,15 +4,6 @@ import type { HostTaskProjectMutationOperations } from './host-task-project-mutation-operations' import type { RpcRequestSender } from '../transport/rpc-client' -import { - fetchAddIssueComment, - fetchAddPRReviewCommentReply, - fetchMergePR, - fetchRequestPRReviewers, - fetchRerunPRChecks, - fetchResolveReviewThread, - type GitHubPrMutationOutcome -} from '../session/github-pr-mutations' const PROJECT_PR_MUTATION_TIMEOUT_MS = 60_000 /** Every project mutation carried a connect deadline before this seam existed. Without one the @@ -87,89 +78,84 @@ export function nativeHostTaskProjectMutationOperations( }) }, async resolveReviewThread(target, repoId, threadId, resolve) { - requirePrMutation( - await fetchResolveReviewThread( - client, - repoId, - { - threadId, - resolve, - // Why: a draft row - // has no slug — send it only when one resolved rather than an empty pair. - prRepo: prRepoPayload(target) - }, - { timeoutMs: PROJECT_MUTATION_TIMEOUT_MS } - ), - resolve ? 'Failed to resolve thread' : 'Failed to reopen thread' + const response = await client.sendRequest( + 'github.resolveReviewThread', + { + repo: `id:${repoId}`, + prRepo: prRepoPayload(target), + threadId, + resolve + }, + { timeoutMs: PROJECT_MUTATION_TIMEOUT_MS } ) + if (!response.ok) { + throw new Error(response.error.message) + } + if (response.result !== true) { + throw new Error(resolve ? 'Failed to resolve thread' : 'Failed to reopen thread') + } }, async replyReviewComment(target, repoId, payload) { - return prMutationComment( - await fetchAddPRReviewCommentReply( - client, - repoId, - { - prNumber: target.number, - ...payload, - prRepo: prRepoPayload(target) - }, - { timeoutMs: PROJECT_MUTATION_TIMEOUT_MS } - ), - 'Failed to reply' + const result = await projectMutation<{ comment?: DetailComment }>( + client, + 'github.addPRReviewCommentReply', + { + repo: `id:${repoId}`, + prNumber: target.number, + prRepo: prRepoPayload(target), + ...payload + } ) + return result.comment }, async addConversationComment(target, repoId, body) { - return prMutationComment( - await fetchAddIssueComment( - client, - repoId, - { - prNumber: target.number, - body, - prRepo: prRepoPayload(target), - type: target.type - }, - { timeoutMs: PROJECT_MUTATION_TIMEOUT_MS } - ), - 'Failed to reply' + const result = await projectMutation<{ comment?: DetailComment }>( + client, + 'github.addIssueComment', + { + repo: `id:${repoId}`, + number: target.number, + prRepo: prRepoPayload(target), + body, + type: target.type + } ) + return result.comment }, async requestReviewers(target, repoId, reviewers) { - requirePrMutation( - await fetchRequestPRReviewers( - client, - repoId, - { - prNumber: target.number, - reviewers, - prRepo: prRepoPayload(target) - }, - { timeoutMs: PROJECT_MUTATION_TIMEOUT_MS } - ), - 'Failed to request reviewers' - ) + await projectMutation(client, 'github.requestPRReviewers', { + repo: `id:${repoId}`, + prNumber: target.number, + prRepo: prRepoPayload(target), + reviewers + }) }, async rerunChecks(target, repoId, payload) { - requirePrMutation( - await fetchRerunPRChecks( - client, - repoId, - { prNumber: target.number, ...payload, prRepo: prRepoPayload(target) }, - // A CI rerun and a merge both routinely outrun the 30s default. - { timeoutMs: PROJECT_PR_MUTATION_TIMEOUT_MS } - ), - 'Failed to rerun checks' + await projectMutation( + client, + 'github.rerunPRChecks', + { + repo: `id:${repoId}`, + prNumber: target.number, + prRepo: prRepoPayload(target), + ...payload + }, + false, + PROJECT_PR_MUTATION_TIMEOUT_MS ) }, async merge(target, repoId, method) { - requirePrMutation( - await fetchMergePR( - client, - repoId, - { prNumber: target.number, method, prRepo: prRepoPayload(target) }, - { timeoutMs: PROJECT_PR_MUTATION_TIMEOUT_MS } - ), - 'Failed to merge pull request' + await projectMutation( + client, + 'github.mergePR', + { + repo: `id:${repoId}`, + prNumber: target.number, + prRepo: prRepoPayload(target), + method + }, + false, + PROJECT_PR_MUTATION_TIMEOUT_MS ) } } @@ -202,49 +188,40 @@ const PROJECT_MUTATION_FALLBACKS: Record = { 'github.project.deleteIssueCommentBySlug': 'Failed to delete comment', 'github.project.updateItemField': 'Failed to update project field', 'github.project.clearItemField': 'Failed to update project field', - 'github.project.updateIssueTypeBySlug': 'Failed to update issue type' + 'github.project.updateIssueTypeBySlug': 'Failed to update issue type', + 'github.addPRReviewCommentReply': 'Failed to reply', + 'github.addIssueComment': 'Failed to reply', + 'github.requestPRReviewers': 'Failed to request reviewers', + 'github.rerunPRChecks': 'Failed to rerun checks', + 'github.mergePR': 'Failed to merge pull request' } async function projectMutation( client: RpcRequestSender, method: string, payload: object, - requireOk = false + requireOk = false, + timeoutMs = PROJECT_MUTATION_TIMEOUT_MS ): Promise { const fallback = PROJECT_MUTATION_FALLBACKS[method] ?? 'GitHub Project request failed' - const response = (await client.sendRequest(method, payload, { timeoutMs: 30_000 })) as { - ok: boolean - result?: { ok?: boolean; error?: string | { message?: string } } - error?: { message?: string } - } + const response = await client.sendRequest(method, payload, { timeoutMs }) if (!response.ok) { - throw new Error(response.error?.message ?? fallback) + throw new Error(response.error.message) } - if (requireOk && !response.result?.ok) { - throw new Error(fallback) + const result = response.result as { ok?: boolean; error?: string | { message?: string } } + if (requireOk ? !result.ok : result.ok === false) { + const error = result.error + if (!method.startsWith('github.project.')) { + throw new Error((error as string | undefined) ?? fallback) + } + const acceptsString = + method === 'github.project.updateIssueCommentBySlug' || + method === 'github.project.deleteIssueCommentBySlug' + throw new Error( + acceptsString && typeof error === 'string' + ? error + : ((typeof error === 'object' ? error?.message : undefined) ?? fallback) + ) } - if (response.result?.ok === false) { - const error = response.result.error - throw new Error(typeof error === 'string' ? error : (error?.message ?? fallback)) - } - return (response.result ?? {}) as T -} - -/** The wrapper substitutes its own copy for two cases the caller used to word itself: a host - * that says nothing (`Request failed: `) and a review thread it could not update. */ -const WRAPPER_SUBSTITUTED_COPY = ['Request failed: ', 'Failed to update review thread.'] - -function requirePrMutation(result: GitHubPrMutationOutcome, fallback: string): void { - if (!result.ok) { - const substituted = WRAPPER_SUBSTITUTED_COPY.some((copy) => result.error.startsWith(copy)) - throw new Error(substituted ? fallback : result.error) - } -} - -function prMutationComment( - result: GitHubPrMutationOutcome, - fallback: string -): DetailComment | undefined { - requirePrMutation(result, fallback) - return (result as { comment?: DetailComment }).comment + return result as T }