fix: address review findings (#2339)
This commit is contained in:
parent
05286ce00b
commit
9bbfb04a09
|
|
@ -2688,6 +2688,38 @@ export async function requestPRReviewers(
|
|||
}
|
||||
}
|
||||
|
||||
export async function removePRReviewers(
|
||||
repoPath: string,
|
||||
prNumber: number,
|
||||
reviewers: string[],
|
||||
connectionId?: string | null
|
||||
): Promise<{ ok: true } | { ok: false; error: string }> {
|
||||
const logins = reviewers.map((reviewer) => reviewer.trim()).filter(Boolean)
|
||||
if (logins.length === 0) {
|
||||
return { ok: false, error: 'Enter at least one reviewer' }
|
||||
}
|
||||
const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId))
|
||||
const ownerRepo = await getOwnerRepo(repoPath, connectionId)
|
||||
await acquire()
|
||||
try {
|
||||
const args = ['pr', 'edit', String(prNumber), '--remove-reviewer', logins.join(',')]
|
||||
if (ownerRepo) {
|
||||
args.push('--repo', `${ownerRepo.owner}/${ownerRepo.repo}`)
|
||||
}
|
||||
await ghExecFileAsync(args, {
|
||||
...ghOptions,
|
||||
env: { ...process.env, GH_PROMPT_DISABLED: '1' }
|
||||
})
|
||||
return { ok: true }
|
||||
} catch (err) {
|
||||
const message =
|
||||
err instanceof Error ? err.message : typeof err === 'string' ? err : 'Unknown error'
|
||||
return { ok: false, error: message }
|
||||
} finally {
|
||||
release()
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Update a PR's title.
|
||||
*/
|
||||
|
|
|
|||
|
|
@ -27,7 +27,7 @@ vi.mock('./gh-utils', async () => {
|
|||
}
|
||||
})
|
||||
|
||||
import { createIssue, getIssue, listIssues } from './issues'
|
||||
import { createIssue, getIssue, listIssues, updateIssue } from './issues'
|
||||
|
||||
describe('issue source operations', () => {
|
||||
beforeEach(() => {
|
||||
|
|
@ -125,4 +125,17 @@ describe('issue source operations', () => {
|
|||
{ cwd: '/repo-root' }
|
||||
)
|
||||
})
|
||||
|
||||
it('updates issue body through the REST issue endpoint', async () => {
|
||||
getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'stablyai', repo: 'orca' })
|
||||
ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: '' })
|
||||
|
||||
await expect(updateIssue('/repo-root', 924, { body: 'Updated body' })).resolves.toEqual({
|
||||
ok: true
|
||||
})
|
||||
expect(ghExecFileAsyncMock).toHaveBeenCalledWith(
|
||||
['api', '-X', 'PATCH', 'repos/stablyai/orca/issues/924', '--raw-field', 'body=Updated body'],
|
||||
{ cwd: '/repo-root' }
|
||||
)
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -242,6 +242,28 @@ export async function updateIssue(
|
|||
}
|
||||
}
|
||||
|
||||
if (updates.body !== undefined) {
|
||||
await acquire()
|
||||
try {
|
||||
await ghExecFileAsync(
|
||||
[
|
||||
'api',
|
||||
'-X',
|
||||
'PATCH',
|
||||
`repos/${ownerRepo.owner}/${ownerRepo.repo}/issues/${issueNumber}`,
|
||||
'--raw-field',
|
||||
`body=${updates.body}`
|
||||
],
|
||||
ghOptions
|
||||
)
|
||||
} catch (err) {
|
||||
const stderr = err instanceof Error ? err.message : String(err)
|
||||
errors.push(classifyGhError(stderr).message)
|
||||
} finally {
|
||||
release()
|
||||
}
|
||||
}
|
||||
|
||||
// Field edits (labels, assignees, title) via gh issue edit
|
||||
const editArgs: string[] = ['issue', 'edit', String(issueNumber), '--repo', repo]
|
||||
let hasEditArgs = false
|
||||
|
|
|
|||
|
|
@ -39,6 +39,7 @@ import {
|
|||
updatePRState,
|
||||
rerunPRChecks,
|
||||
requestPRReviewers,
|
||||
removePRReviewers,
|
||||
checkOrcaStarred,
|
||||
starOrca
|
||||
} from '../github/client'
|
||||
|
|
@ -646,6 +647,26 @@ export function registerGitHubHandlers(store: Store, stats: StatsCollector): voi
|
|||
}
|
||||
)
|
||||
|
||||
ipcMain.handle(
|
||||
'gh:removePRReviewers',
|
||||
async (event, args: { repoPath: string; prNumber: number; reviewers: string[] }) => {
|
||||
const repo = assertRegisteredRepo(args, store)
|
||||
const result = await removePRReviewers(
|
||||
repo.path,
|
||||
args.prNumber,
|
||||
args.reviewers,
|
||||
repoConnectionId(repo)
|
||||
)
|
||||
if (result.ok) {
|
||||
broadcastWorkItemMutated(
|
||||
{ repoPath: repo.path, repoId: repo.id, type: 'pr', number: args.prNumber },
|
||||
event.sender.id
|
||||
)
|
||||
}
|
||||
return result
|
||||
}
|
||||
)
|
||||
|
||||
ipcMain.handle(
|
||||
'gh:updateIssue',
|
||||
async (
|
||||
|
|
|
|||
|
|
@ -135,6 +135,7 @@ import {
|
|||
mergePR,
|
||||
updatePRState,
|
||||
requestPRReviewers,
|
||||
removePRReviewers,
|
||||
createIssue,
|
||||
updateIssue,
|
||||
addIssueComment,
|
||||
|
|
@ -5303,6 +5304,16 @@ export class OrcaRuntimeService {
|
|||
return requestPRReviewers(repo.path, prNumber, reviewers)
|
||||
}
|
||||
|
||||
async removeRepoPRReviewers(
|
||||
repoSelector: string,
|
||||
prNumber: number,
|
||||
reviewers: string[]
|
||||
): Promise<Awaited<ReturnType<typeof removePRReviewers>>> {
|
||||
const repo = await this.resolveRepoSelector(repoSelector)
|
||||
this.assertHostIntegrationRepoIsLocal(repo, 'repo_pr_reviewers')
|
||||
return removePRReviewers(repo.path, prNumber, reviewers)
|
||||
}
|
||||
|
||||
async createRepoIssue(
|
||||
repoSelector: string,
|
||||
title: string,
|
||||
|
|
|
|||
|
|
@ -120,6 +120,11 @@ const RequestPrReviewers = RepoSelector.extend({
|
|||
reviewers: z.array(z.string()).min(1)
|
||||
})
|
||||
|
||||
const RemovePrReviewers = RepoSelector.extend({
|
||||
prNumber: z.number().int().positive(),
|
||||
reviewers: z.array(z.string()).min(1)
|
||||
})
|
||||
|
||||
const CreateIssue = RepoSelector.extend({
|
||||
title: requiredString('Missing title'),
|
||||
body: z.string()
|
||||
|
|
@ -405,6 +410,12 @@ export const GITHUB_METHODS: RpcMethod[] = [
|
|||
handler: async (params, { runtime }) =>
|
||||
runtime.requestRepoPRReviewers(params.repo, params.prNumber, params.reviewers)
|
||||
}),
|
||||
defineMethod({
|
||||
name: 'github.removePRReviewers',
|
||||
params: RemovePrReviewers,
|
||||
handler: async (params, { runtime }) =>
|
||||
runtime.removeRepoPRReviewers(params.repo, params.prNumber, params.reviewers)
|
||||
}),
|
||||
defineMethod({
|
||||
name: 'github.createIssue',
|
||||
params: CreateIssue,
|
||||
|
|
|
|||
|
|
@ -889,6 +889,12 @@ export type PreloadApi = {
|
|||
prNumber: number
|
||||
reviewers: string[]
|
||||
}) => Promise<{ ok: true } | { ok: false; error: string }>
|
||||
removePRReviewers: (args: {
|
||||
repoPath: string
|
||||
repoId?: string
|
||||
prNumber: number
|
||||
reviewers: string[]
|
||||
}) => Promise<{ ok: true } | { ok: false; error: string }>
|
||||
updateIssue: (args: {
|
||||
repoPath: string
|
||||
repoId?: string
|
||||
|
|
|
|||
|
|
@ -915,6 +915,14 @@ const api = {
|
|||
}): Promise<{ ok: true } | { ok: false; error: string }> =>
|
||||
ipcRenderer.invoke('gh:requestPRReviewers', args),
|
||||
|
||||
removePRReviewers: (args: {
|
||||
repoPath: string
|
||||
repoId?: string
|
||||
prNumber: number
|
||||
reviewers: string[]
|
||||
}): Promise<{ ok: true } | { ok: false; error: string }> =>
|
||||
ipcRenderer.invoke('gh:removePRReviewers', args),
|
||||
|
||||
updateIssue: (args: {
|
||||
repoPath: string
|
||||
repoId?: string
|
||||
|
|
|
|||
File diff suppressed because it is too large
Load Diff
|
|
@ -3,6 +3,45 @@ import { describe, expect, it } from 'vitest'
|
|||
import CommentMarkdown from './CommentMarkdown'
|
||||
|
||||
describe('CommentMarkdown', () => {
|
||||
it('autolinks same-repo GitHub issue references when repo context is provided', () => {
|
||||
const markup = renderToStaticMarkup(
|
||||
<CommentMarkdown
|
||||
variant="document"
|
||||
githubRepo={{ owner: 'stablyai', repo: 'orca' }}
|
||||
content="Automated fix-PR from pr-bug-scan for parent **#2316**."
|
||||
/>
|
||||
)
|
||||
|
||||
expect(markup).toContain('href="https://github.com/stablyai/orca/issues/2316"')
|
||||
expect(markup).toContain('<strong><a')
|
||||
})
|
||||
|
||||
it('autolinks cross-repo GitHub issue references', () => {
|
||||
const markup = renderToStaticMarkup(
|
||||
<CommentMarkdown
|
||||
variant="document"
|
||||
githubRepo={{ owner: 'stablyai', repo: 'orca' }}
|
||||
content="See another-org/other-repo#42."
|
||||
/>
|
||||
)
|
||||
|
||||
expect(markup).toContain('href="https://github.com/another-org/other-repo/issues/42"')
|
||||
})
|
||||
|
||||
it('does not autolink GitHub issue references inside existing links or code', () => {
|
||||
const markup = renderToStaticMarkup(
|
||||
<CommentMarkdown
|
||||
variant="document"
|
||||
githubRepo={{ owner: 'stablyai', repo: 'orca' }}
|
||||
content="[`#2316`](https://example.com/already-linked) and `#2317`"
|
||||
/>
|
||||
)
|
||||
|
||||
expect(markup).toContain('href="https://example.com/already-linked"')
|
||||
expect(markup).not.toContain('href="https://github.com/stablyai/orca/issues/2316"')
|
||||
expect(markup).not.toContain('href="https://github.com/stablyai/orca/issues/2317"')
|
||||
})
|
||||
|
||||
it('contains long PR body markdown inside its available width', () => {
|
||||
const markup = renderToStaticMarkup(
|
||||
<CommentMarkdown
|
||||
|
|
|
|||
|
|
@ -9,6 +9,29 @@ import { cn } from '@/lib/utils'
|
|||
|
||||
type MarkdownPlugins = NonNullable<React.ComponentProps<typeof Markdown>['rehypePlugins']>
|
||||
|
||||
type GitHubRepoReference = {
|
||||
owner: string
|
||||
repo: string
|
||||
}
|
||||
|
||||
type MarkdownTextNode = {
|
||||
type: 'text'
|
||||
value: string
|
||||
}
|
||||
|
||||
type MarkdownLinkNode = {
|
||||
type: 'link'
|
||||
url: string
|
||||
title: null
|
||||
children: MarkdownTextNode[]
|
||||
}
|
||||
|
||||
type MarkdownNode = {
|
||||
type: string
|
||||
value?: string
|
||||
children?: MarkdownNode[]
|
||||
}
|
||||
|
||||
// Why: sidebar comments are rendered at 11px in a narrow card, so we strip
|
||||
// block-level wrappers that add unwanted margins and only keep inline
|
||||
// formatting (bold, italic, code, links) plus compact lists and line breaks.
|
||||
|
|
@ -170,6 +193,94 @@ const documentComponents: Components = {
|
|||
// with existing plain-text comments that rely on newline formatting.
|
||||
const remarkPlugins = [remarkGfm, remarkBreaks]
|
||||
|
||||
const GITHUB_REFERENCE_PATTERN = /(?:\b([A-Za-z0-9_.-]+)\/([A-Za-z0-9_.-]+))?#([1-9][0-9]*)\b/g
|
||||
|
||||
function createGitHubIssueUrl(owner: string, repo: string, number: string): string {
|
||||
return `https://github.com/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/issues/${number}`
|
||||
}
|
||||
|
||||
function isEmbeddedGitHubReference(value: string, index: number): boolean {
|
||||
if (index === 0) {
|
||||
return false
|
||||
}
|
||||
return /[A-Za-z0-9_./-]/.test(value[index - 1] ?? '')
|
||||
}
|
||||
|
||||
function createGitHubReferenceLinkNode(
|
||||
label: string,
|
||||
owner: string,
|
||||
repo: string,
|
||||
number: string
|
||||
): MarkdownLinkNode {
|
||||
return {
|
||||
type: 'link',
|
||||
url: createGitHubIssueUrl(owner, repo, number),
|
||||
title: null,
|
||||
children: [{ type: 'text', value: label }]
|
||||
}
|
||||
}
|
||||
|
||||
function splitGitHubReferenceText(value: string, defaultRepo: GitHubRepoReference): MarkdownNode[] {
|
||||
const parts: MarkdownNode[] = []
|
||||
let cursor = 0
|
||||
|
||||
for (const match of value.matchAll(GITHUB_REFERENCE_PATTERN)) {
|
||||
const label = match[0]
|
||||
const index = match.index ?? 0
|
||||
if (isEmbeddedGitHubReference(value, index)) {
|
||||
continue
|
||||
}
|
||||
|
||||
const owner = match[1] ?? defaultRepo.owner
|
||||
const repo = match[2] ?? defaultRepo.repo
|
||||
const number = match[3]
|
||||
if (!number) {
|
||||
continue
|
||||
}
|
||||
|
||||
if (index > cursor) {
|
||||
parts.push({ type: 'text', value: value.slice(cursor, index) })
|
||||
}
|
||||
parts.push(createGitHubReferenceLinkNode(label, owner, repo, number))
|
||||
cursor = index + label.length
|
||||
}
|
||||
|
||||
if (cursor === 0) {
|
||||
return [{ type: 'text', value }]
|
||||
}
|
||||
if (cursor < value.length) {
|
||||
parts.push({ type: 'text', value: value.slice(cursor) })
|
||||
}
|
||||
return parts
|
||||
}
|
||||
|
||||
function transformGitHubReferenceChildren(
|
||||
node: MarkdownNode,
|
||||
defaultRepo: GitHubRepoReference
|
||||
): void {
|
||||
if (!node.children || node.type === 'link' || node.type === 'image') {
|
||||
return
|
||||
}
|
||||
|
||||
const nextChildren: MarkdownNode[] = []
|
||||
for (const child of node.children) {
|
||||
if (child.type === 'text' && child.value !== undefined) {
|
||||
nextChildren.push(...splitGitHubReferenceText(child.value, defaultRepo))
|
||||
} else {
|
||||
transformGitHubReferenceChildren(child, defaultRepo)
|
||||
nextChildren.push(child)
|
||||
}
|
||||
}
|
||||
|
||||
node.children = nextChildren
|
||||
}
|
||||
|
||||
export function remarkGitHubReferences(
|
||||
defaultRepo: GitHubRepoReference
|
||||
): () => (tree: MarkdownNode) => void {
|
||||
return () => (tree) => transformGitHubReferenceChildren(tree, defaultRepo)
|
||||
}
|
||||
|
||||
const commentMarkdownSanitizeSchema = {
|
||||
...defaultSchema,
|
||||
tagNames: [...(defaultSchema.tagNames ?? []), 'details', 'summary', 'sub', 'sup', 'ins', 'kbd'],
|
||||
|
|
@ -191,6 +302,7 @@ const rehypePlugins: MarkdownPlugins = [rehypeRaw, [rehypeSanitize, commentMarkd
|
|||
type CommentMarkdownProps = React.ComponentPropsWithoutRef<'div'> & {
|
||||
content: string
|
||||
variant?: 'compact' | 'document'
|
||||
githubRepo?: GitHubRepoReference | null
|
||||
}
|
||||
|
||||
// Why forwardRef + rest props: Radix's HoverCardTrigger asChild merges a ref
|
||||
|
|
@ -198,10 +310,14 @@ type CommentMarkdownProps = React.ComponentPropsWithoutRef<'div'> & {
|
|||
// the child. Without forwarding both, the hover card cannot open or position.
|
||||
const CommentMarkdown = React.memo(
|
||||
React.forwardRef<HTMLDivElement, CommentMarkdownProps>(function CommentMarkdown(
|
||||
{ content, className, variant = 'compact', ...rest },
|
||||
{ content, className, variant = 'compact', githubRepo, ...rest },
|
||||
ref
|
||||
) {
|
||||
const components = variant === 'document' ? documentComponents : compactComponents
|
||||
const activeRemarkPlugins = React.useMemo(
|
||||
() => (githubRepo ? [...remarkPlugins, remarkGitHubReferences(githubRepo)] : remarkPlugins),
|
||||
[githubRepo]
|
||||
)
|
||||
|
||||
return (
|
||||
<div
|
||||
|
|
@ -217,7 +333,7 @@ const CommentMarkdown = React.memo(
|
|||
{...rest}
|
||||
>
|
||||
<Markdown
|
||||
remarkPlugins={remarkPlugins}
|
||||
remarkPlugins={activeRemarkPlugins}
|
||||
rehypePlugins={rehypePlugins}
|
||||
components={components}
|
||||
>
|
||||
|
|
|
|||
|
|
@ -705,6 +705,7 @@ function createGitHubApi(): NonNullable<Partial<PreloadApi>['gh']> {
|
|||
mergePR: direct('github.mergePR'),
|
||||
updatePRState: direct('github.updatePRState'),
|
||||
requestPRReviewers: direct('github.requestPRReviewers'),
|
||||
removePRReviewers: direct('github.removePRReviewers'),
|
||||
updateIssue: direct('github.updateIssue'),
|
||||
addIssueComment: direct('github.addIssueComment'),
|
||||
addPRReviewCommentReply: direct('github.addPRReviewCommentReply'),
|
||||
|
|
|
|||
|
|
@ -916,10 +916,9 @@ export type LinearComment = {
|
|||
export type GitHubIssueUpdate = {
|
||||
state?: 'open' | 'closed'
|
||||
title?: string
|
||||
// Why: body writes are driven by the Project-mode slug-addressed path
|
||||
// (`updateIssueBySlug`) because `gh issue edit` does not consistently
|
||||
// cover every body-edit case the dialog needs; the repoPath-based
|
||||
// `updateIssue` flow keeps ignoring `body` for backward compatibility.
|
||||
// Why: body writes use the REST issue endpoint instead of `gh issue edit`
|
||||
// because that command does not consistently cover every body-edit case the
|
||||
// dialog needs.
|
||||
body?: string
|
||||
addLabels?: string[]
|
||||
removeLabels?: string[]
|
||||
|
|
|
|||
Loading…
Reference in New Issue