From c98504b81a35c0520e034d61114902fc941da6bc Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sun, 16 Aug 2026 19:21:30 -0700 Subject: [PATCH] Improve PR page error messages, accessibility, and stability - Show actual error messages from failed operations instead of generic fallbacks - Add aria attributes for combobox/listbox patterns and proper option identifiers - Consolidate duplicate label/assignee update logic and fix event listener passive mode - Memoize GitHub source runtime to prevent stale closure in checks callbacks - Extract filled state badge tone for reuse and fix workspace attachment type - Guard textarea shortcuts against concurrent saves and add mention query test --- .../pull-request-page-host-boundary.test.ts | 4 +- .../actions/merge-actions.ts | 16 ++-- .../pull-request-page/checks/tab.tsx | 17 +++- .../conversation/comment-card.tsx | 1 - .../conversation/description.tsx | 4 +- .../pull-request-page/edit/issue-updates.ts | 94 +++---------------- .../pull-request-page/files/toolbar.tsx | 16 ++-- .../pull-request-page/files/view-restore.ts | 2 +- .../pull-request-page/mentions/query.test.ts | 4 + .../pull-request-page/mentions/textarea.tsx | 16 +++- .../pull-request-page/page/header.tsx | 23 ++--- .../pull-request-page/page/surface.tsx | 22 ++--- .../pull-request-page/page/tabs-shell.tsx | 13 ++- .../presentation/state-badge.test.ts | 16 +++- .../presentation/state-badge.tsx | 13 +++ .../reviewers/picker-row.tsx | 3 + .../pull-request-page/reviewers/picker.tsx | 14 ++- .../reviewers/request-actions.ts | 10 +- 18 files changed, 147 insertions(+), 141 deletions(-) diff --git a/src/renderer/src/components/pull-request-page-host-boundary.test.ts b/src/renderer/src/components/pull-request-page-host-boundary.test.ts index a90a50b132e..98a6c2521a2 100644 --- a/src/renderer/src/components/pull-request-page-host-boundary.test.ts +++ b/src/renderer/src/components/pull-request-page-host-boundary.test.ts @@ -122,9 +122,7 @@ describe('PullRequestPage host boundaries', () => { expect(editSource).toContain( 'args.patchWorkItem(args.item.id, { state: args.newState }, args.item.repoId, {' ) - expect(editSource).toContain( - 'args.patchWorkItem(args.item.id, { labels: newLabels }, args.item.repoId, {' - ) + expect(editSource).toContain('args.patchWorkItem(args.item.id, { labels }, args.item.repoId, {') }) it('routes PR mention metadata through the PR repo owner host', () => { diff --git a/src/renderer/src/components/pull-request-page/actions/merge-actions.ts b/src/renderer/src/components/pull-request-page/actions/merge-actions.ts index 54e2728cc9f..27bd98d963c 100644 --- a/src/renderer/src/components/pull-request-page/actions/merge-actions.ts +++ b/src/renderer/src/components/pull-request-page/actions/merge-actions.ts @@ -183,9 +183,11 @@ export async function mergePullRequest(args: { } toast.success(translate('auto.components.PullRequestPage.c57873d721', 'Pull request merged')) args.onMutated() - } catch { + } catch (err) { toast.error( - translate('auto.components.PullRequestPage.aae645d36d', 'Failed to merge pull request') + err instanceof Error + ? err.message + : translate('auto.components.PullRequestPage.aae645d36d', 'Failed to merge pull request') ) } finally { args.setMergePending(false) @@ -256,11 +258,13 @@ export async function setPullRequestAutoMerge(args: { : translate('auto.components.PullRequestPage.0f5821b035', 'Auto-merge disabled') ) args.onMutated() - } catch { + } catch (err) { toast.error( - enabled - ? translate('auto.components.PullRequestPage.d31f4b508c', 'Failed to enable auto-merge') - : translate('auto.components.PullRequestPage.973ef2fac9', 'Failed to disable auto-merge') + err instanceof Error + ? err.message + : enabled + ? translate('auto.components.PullRequestPage.d31f4b508c', 'Failed to enable auto-merge') + : translate('auto.components.PullRequestPage.973ef2fac9', 'Failed to disable auto-merge') ) } finally { args.setMergePending(false) diff --git a/src/renderer/src/components/pull-request-page/checks/tab.tsx b/src/renderer/src/components/pull-request-page/checks/tab.tsx index 9efc13d49ab..59e87cd0b87 100644 --- a/src/renderer/src/components/pull-request-page/checks/tab.tsx +++ b/src/renderer/src/components/pull-request-page/checks/tab.tsx @@ -66,6 +66,8 @@ export function ChecksTab({ const repo = useAppStore((s) => targetRepoId ? (s.repos.find((candidate) => candidate.id === targetRepoId) ?? null) : null ) + const repos = useAppStore((s) => s.repos) + const projects = useAppStore((s) => s.projects) const [fixingChecks, setFixingChecks] = useState(false) const [fixChecksComposerPrompt, setFixChecksComposerPrompt] = useState(null) const mountedRef = useMountedRef() @@ -124,14 +126,23 @@ export function ChecksTab({ projectRuntime: repo?.connectionId ? undefined : getLocalRepoProjectExecutionRuntimeContext( - useAppStore.getState(), + // Why: repo-scoped resolution only reads repos/projects/settings, and subscribing keeps the launch platform fresh. + { + activeRepoId: null, + activeWorktreeId: null, + projects, + repos, + settings, + worktreesByRepo: {} + }, repo?.id, CLIENT_PLATFORM ) }), - [repo?.connectionId, repo?.id, repo?.path] + [projects, repo?.connectionId, repo?.id, repo?.path, repos, settings] ) - const runtimeHost = getGitHubSourceRuntimeHost(sourceContext) + // Why: parses a fresh host object per call, so memoize to keep the check action callbacks stable. + const runtimeHost = useMemo(() => getGitHubSourceRuntimeHost(sourceContext), [sourceContext]) const canUseChecksRepoContext = canUseGitHubRepoContext(repoPath, sourceContext) const sorted = sortChecksBySeverity(list) const failedChecks = getBrokenChecks(list) diff --git a/src/renderer/src/components/pull-request-page/conversation/comment-card.tsx b/src/renderer/src/components/pull-request-page/conversation/comment-card.tsx index e4b541b494a..66ed253a6c3 100644 --- a/src/renderer/src/components/pull-request-page/conversation/comment-card.tsx +++ b/src/renderer/src/components/pull-request-page/conversation/comment-card.tsx @@ -55,7 +55,6 @@ export function ConversationCommentCard({ }): React.JSX.Element { return (
{ - if (event.key === 'Escape') { + if (event.key === 'Escape' && !bodySaving) { event.preventDefault() onCancelEdit() return } - if (isScreenSubmitShortcut(event)) { + if (isScreenSubmitShortcut(event) && !bodySaving && bodyChanged) { event.preventDefault() onSave() } diff --git a/src/renderer/src/components/pull-request-page/edit/issue-updates.ts b/src/renderer/src/components/pull-request-page/edit/issue-updates.ts index 433e4c0faa0..044b1e6f23c 100644 --- a/src/renderer/src/components/pull-request-page/edit/issue-updates.ts +++ b/src/renderer/src/components/pull-request-page/edit/issue-updates.ts @@ -96,37 +96,12 @@ export function toggleIssueLabel(args: { ? [...prevLabels, args.label] : prevLabels.filter((l) => l !== args.label) - if (isAdding) { - args.run('labels', { - mutate: () => - runIssueUpdate({ - repoId: args.item.repoId, - repoPath: args.repoPath, - sourceContext: args.sourceContext, - projectOrigin: args.projectOrigin, - number: args.item.number, - updates: { addLabels: [args.label] } - }), - onOptimistic: () => { - args.onLabelsChange(newLabels) - args.patchWorkItem(args.item.id, { labels: newLabels }, args.item.repoId, { - sourceContext: args.sourceContext - }) - args.patchProjectRowIfNeeded({ labels: newLabels }) - }, - onSuccess: () => { - args.onMutated() - }, - onRevert: () => { - args.onLabelsChange(prevLabels) - args.patchWorkItem(args.item.id, { labels: prevLabels }, args.item.repoId, { - sourceContext: args.sourceContext - }) - args.patchProjectRowIfNeeded({ labels: prevLabels }) - }, - onError: (err) => toast.error(err) + const applyLabels = (labels: string[]): void => { + args.onLabelsChange(labels) + args.patchWorkItem(args.item.id, { labels }, args.item.repoId, { + sourceContext: args.sourceContext }) - return + args.patchProjectRowIfNeeded({ labels }) } args.run('labels', { @@ -137,22 +112,10 @@ export function toggleIssueLabel(args: { sourceContext: args.sourceContext, projectOrigin: args.projectOrigin, number: args.item.number, - updates: { removeLabels: [args.label] } + updates: isAdding ? { addLabels: [args.label] } : { removeLabels: [args.label] } }), - onOptimistic: () => { - args.onLabelsChange(newLabels) - args.patchWorkItem(args.item.id, { labels: newLabels }, args.item.repoId, { - sourceContext: args.sourceContext - }) - args.patchProjectRowIfNeeded({ labels: newLabels }) - }, - onRevert: () => { - args.onLabelsChange(prevLabels) - args.patchWorkItem(args.item.id, { labels: prevLabels }, args.item.repoId, { - sourceContext: args.sourceContext - }) - args.patchProjectRowIfNeeded({ labels: prevLabels }) - }, + onOptimistic: () => applyLabels(newLabels), + onRevert: () => applyLabels(prevLabels), onSuccess: () => { args.onMutated() }, @@ -182,31 +145,10 @@ export function toggleIssueAssignee(args: { // Why: scope the optimistic guard to this repo item so switching items doesn't suppress the next item's assignee sync. args.editedAssigneesItemKeyRef.current = args.assigneesItemKey - if (isAssigned) { - args.run('assignees', { - mutate: () => - runIssueUpdate({ - repoId: args.item.repoId, - repoPath: args.repoPath, - sourceContext: args.sourceContext, - projectOrigin: args.projectOrigin, - number: args.item.number, - updates: { removeAssignees: [args.login] } - }), - onOptimistic: () => { - args.setLocalAssignees(newAssignees) - args.patchProjectRowIfNeeded({ assignees: newAssignees }) - }, - onRevert: () => { - args.setLocalAssignees(prevAssignees) - args.patchProjectRowIfNeeded({ assignees: prevAssignees }) - }, - onSuccess: () => { - args.onMutated() - }, - onError: (err) => toast.error(err) - }) - return + + const applyAssignees = (assignees: string[]): void => { + args.setLocalAssignees(assignees) + args.patchProjectRowIfNeeded({ assignees }) } args.run('assignees', { @@ -217,19 +159,13 @@ export function toggleIssueAssignee(args: { sourceContext: args.sourceContext, projectOrigin: args.projectOrigin, number: args.item.number, - updates: { addAssignees: [args.login] } + updates: isAssigned ? { removeAssignees: [args.login] } : { addAssignees: [args.login] } }), - onOptimistic: () => { - args.setLocalAssignees(newAssignees) - args.patchProjectRowIfNeeded({ assignees: newAssignees }) - }, + onOptimistic: () => applyAssignees(newAssignees), + onRevert: () => applyAssignees(prevAssignees), onSuccess: () => { args.onMutated() }, - onRevert: () => { - args.setLocalAssignees(prevAssignees) - args.patchProjectRowIfNeeded({ assignees: prevAssignees }) - }, onError: (err) => toast.error(err) }) } diff --git a/src/renderer/src/components/pull-request-page/files/toolbar.tsx b/src/renderer/src/components/pull-request-page/files/toolbar.tsx index a27e8938437..216bb5f950c 100644 --- a/src/renderer/src/components/pull-request-page/files/toolbar.tsx +++ b/src/renderer/src/components/pull-request-page/files/toolbar.tsx @@ -53,24 +53,28 @@ export function PRFilesDiffToolbar({
- - +
) diff --git a/src/renderer/src/components/pull-request-page/files/view-restore.ts b/src/renderer/src/components/pull-request-page/files/view-restore.ts index 2f4bcbc2f8e..5662ba30ace 100644 --- a/src/renderer/src/components/pull-request-page/files/view-restore.ts +++ b/src/renderer/src/components/pull-request-page/files/view-restore.ts @@ -77,7 +77,7 @@ export function usePRFilesDiffViewPersistence(args: { writeCachedScrollPosition(args.pendingRestoreScrollTopRef.current ?? container.scrollTop) } - container.addEventListener('scroll', cacheScrollPositionOnScroll) + container.addEventListener('scroll', cacheScrollPositionOnScroll, { passive: true }) return () => { cacheScrollPositionOnTeardown() container.removeEventListener('scroll', cacheScrollPositionOnScroll) diff --git a/src/renderer/src/components/pull-request-page/mentions/query.test.ts b/src/renderer/src/components/pull-request-page/mentions/query.test.ts index 92ed6824b44..5ade202f9a3 100644 --- a/src/renderer/src/components/pull-request-page/mentions/query.test.ts +++ b/src/renderer/src/components/pull-request-page/mentions/query.test.ts @@ -19,4 +19,8 @@ describe('findMentionQuery', () => { it('allows a mention after punctuation that starts a token', () => { expect(findMentionQuery('(@octo', 6)).toEqual({ atIndex: 1, query: 'octo' }) }) + + it('reads only the text before the caret', () => { + expect(findMentionQuery('hi @octo trailing', 8)).toEqual({ atIndex: 3, query: 'octo' }) + }) }) diff --git a/src/renderer/src/components/pull-request-page/mentions/textarea.tsx b/src/renderer/src/components/pull-request-page/mentions/textarea.tsx index a70abe58858..637acad4183 100644 --- a/src/renderer/src/components/pull-request-page/mentions/textarea.tsx +++ b/src/renderer/src/components/pull-request-page/mentions/textarea.tsx @@ -1,4 +1,4 @@ -import React, { useCallback, useMemo, useState } from 'react' +import React, { useCallback, useId, useMemo, useState } from 'react' import { cn } from '@/lib/utils' import { filterGitHubMentionOptions } from '@/components/github/github-mention-option-filter' import type { MentionOption, MentionQuery } from '../page-types' @@ -25,6 +25,7 @@ export function MentionTextarea({ mentionOptions: MentionOption[] textareaRef: React.RefObject }): React.JSX.Element { + const listboxId = useId() const [mentionQuery, setMentionQuery] = useState(null) const [activeIndex, setActiveIndex] = useState(0) const suggestions = useMemo( @@ -64,10 +65,17 @@ export function MentionTextarea({ return (
{showSuggestions && ( -
+
{suggestions.map((option, index) => (