Improve PR page stability: add error handling and fix race conditions

- Add error handling with user feedback (toast notifications) for comment submission, review comments, diff loading, and file view syncing
- Internationalize hardcoded strings for PR state labels and error messages
- Fix race condition in reviewer submission by using a ref-based guard instead of render-time state
- Fix scroll restoration to avoid overwriting target positions with intermediate clamp values
- Add effectiveRepoId parameter for proper repo context in review operations
- Disable reviewer picker during submission to prevent concurrent requests
This commit is contained in:
Jinjing
2026-08-16 19:21:35 -07:00
parent 150e9d53cc
commit 55f98bd240
16 changed files with 110 additions and 38 deletions
@@ -40,7 +40,10 @@ export async function changePullRequestState(args: {
if (!args.canMutateState || args.statePending) {
return
}
const label = args.nextState === 'closed' ? 'Close' : 'Reopen'
const label =
args.nextState === 'closed'
? translate('auto.components.PullRequestPage.77482513f8', 'Close')
: translate('auto.components.PullRequestPage.2f5195c6a0', 'Reopen')
const confirmed = await args.confirm({
title: translate('auto.components.PullRequestPage.eec3706a6a', '{{value0}} PR #{{value1}}?', {
value0: label,
@@ -57,6 +57,14 @@ export function CommentReplyForm({
if (ok) {
setBody('')
}
} catch (error) {
if (mountedRef.current) {
toast.error(
error instanceof Error
? error.message
: translate('auto.components.PullRequestPage.5821aab360', 'Failed to post reply.')
)
}
} finally {
if (mountedRef.current) {
setSubmitting(false)
@@ -28,18 +28,28 @@ export async function addPullRequestLineComment(args: {
)
return false
}
const result = await addPRReviewCommentForRepo({
repoPath: args.repoPath,
repoId: args.repoId,
sourceContext: args.sourceContext,
prNumber: args.prNumber,
prRepo: args.prRepo,
commitId: args.headSha,
path: args.section.path,
line: args.lineNumber,
startLine: args.startLine,
body: args.body
})
let result: Awaited<ReturnType<typeof addPRReviewCommentForRepo>>
try {
result = await addPRReviewCommentForRepo({
repoPath: args.repoPath,
repoId: args.repoId,
sourceContext: args.sourceContext,
prNumber: args.prNumber,
prRepo: args.prRepo,
commitId: args.headSha,
path: args.section.path,
line: args.lineNumber,
startLine: args.startLine,
body: args.body
})
} catch (error) {
toast.error(
error instanceof Error
? error.message
: translate('auto.components.PullRequestPage.19628e058d', 'Failed to add review comment.')
)
return false
}
if (!result.ok) {
toast.error(
result.error ||
@@ -126,13 +126,17 @@ export function usePRFileSectionLoader(args: {
modifiedIsBinary: false
} as GitDiffResult,
resultContents: undefined,
error: error instanceof Error ? error.message : 'Failed to load diff.'
error:
error instanceof Error
? error.message
: translate('auto.components.PullRequestPage.4b8ae7303f', 'Failed to load diff.')
}))
.then(({ result, resultContents, error }) => {
loadingIndicesRef.current.delete(index)
// Why: a generation bump already cleared the set, so a stale load must not evict the new entry.
if (generationRef.current !== generation) {
return
}
loadingIndicesRef.current.delete(index)
const largeDiffRenderLimit =
!error && result.kind === 'text' && resultContents
? getPRFileContentsRenderLimit(resultContents)
@@ -56,6 +56,10 @@ export function usePRFilesDiffViewPersistence(args: {
}
const updateCachedScrollPosition = (): void => {
// Why: mid-restore scrollTop is a clamped intermediate, so caching it would lose the real target.
if (args.pendingRestoreScrollTopRef.current !== null) {
return
}
const existing = prFilesDiffViewStateCache.get(args.viewStateKey)
setWithLRU(prFilesDiffScrollTopCache, args.viewStateKey, container.scrollTop)
if (!existing || existing.entrySignature !== args.entrySignature) {
@@ -72,7 +76,12 @@ export function usePRFilesDiffViewPersistence(args: {
updateCachedScrollPosition()
container.removeEventListener('scroll', updateCachedScrollPosition)
}
}, [args.entrySignature, args.scrollContainerRef, args.viewStateKey])
}, [
args.entrySignature,
args.pendingRestoreScrollTopRef,
args.scrollContainerRef,
args.viewStateKey
])
useLayoutEffect(() => {
const container = args.scrollContainerRef.current
@@ -93,9 +102,10 @@ export function usePRFilesDiffViewPersistence(args: {
const maxScrollTop = Math.max(0, liveContainer.scrollHeight - liveContainer.clientHeight)
const nextScrollTop = Math.min(liveTarget, maxScrollTop)
liveContainer.scrollTop = nextScrollTop
setWithLRU(prFilesDiffScrollTopCache, args.viewStateKey, nextScrollTop)
// Why: only cache once the restore lands; an intermediate clamp would overwrite the real target.
if (Math.abs(liveContainer.scrollTop - liveTarget) <= 1 || maxScrollTop >= liveTarget) {
setWithLRU(prFilesDiffScrollTopCache, args.viewStateKey, nextScrollTop)
args.pendingRestoreScrollTopRef.current = null
return
}
@@ -103,7 +113,10 @@ export function usePRFilesDiffViewPersistence(args: {
attempts += 1
if (attempts < 30) {
frameId = window.requestAnimationFrame(restoreScrollPosition)
return
}
// Why: give up rather than block scroll caching forever when the content never grows enough.
args.pendingRestoreScrollTopRef.current = null
}
restoreScrollPosition()
@@ -286,6 +286,7 @@ export default function PullRequestPage({
canUseDetailsRepoContext,
details,
workItem,
effectiveRepoId,
path,
viewed,
detailsCacheKey,
@@ -298,6 +299,7 @@ export default function PullRequestPage({
canUseDetailsRepoContext,
details,
detailsCacheKey,
effectiveRepoId,
projectOrigin,
repoPath,
sourceContext,
@@ -15,6 +15,7 @@ export async function syncPullRequestFileViewed(args: {
canUseDetailsRepoContext: boolean
details: GitHubWorkItemDetails | null
workItem: GitHubWorkItem | null
effectiveRepoId: string | null
path: string
viewed: boolean
detailsCacheKey: string | null
@@ -42,9 +43,21 @@ export async function syncPullRequestFileViewed(args: {
const previousState = args.detailsCacheKey
? patchCachedPRFileViewedState(args.detailsCacheKey, args.path, nextState)
: undefined
const rollbackWithError = (): false => {
if (args.detailsCacheKey && previousState) {
patchCachedPRFileViewedState(args.detailsCacheKey, args.path, previousState)
}
toast.error(
translate(
'auto.components.PullRequestPage.5a01ca7253',
'Failed to sync viewed state with GitHub.'
)
)
return false
}
try {
const ok = await setPRFileViewedForRepo({
repoId: args.workItem.repoId,
repoId: args.effectiveRepoId ?? args.workItem.repoId,
repoPath: args.repoPath ?? '',
sourceContext: args.sourceContext,
prNumber: args.workItem.number,
@@ -54,18 +67,11 @@ export async function syncPullRequestFileViewed(args: {
viewed: args.viewed
})
if (!ok) {
if (args.detailsCacheKey && previousState) {
patchCachedPRFileViewedState(args.detailsCacheKey, args.path, previousState)
}
toast.error(
translate(
'auto.components.PullRequestPage.5a01ca7253',
'Failed to sync viewed state with GitHub.'
)
)
return false
return rollbackWithError()
}
return true
} catch {
return rollbackWithError()
} finally {
args.setPendingViewedPaths((prev) => {
const next = new Set(prev)
@@ -66,6 +66,7 @@ export function PRReviewersPanel({
: repoOwnerSettings,
[repoOwnerSettings, sourceContext]
)
const submittingRef = useRef(false)
const reviewerInputRef = useRef<HTMLInputElement | null>(null)
const reviewerInputFocusFrameRef = useRef<number | null>(null)
const reviewerPanelMountedRef = useRef(true)
@@ -243,7 +244,7 @@ export function PRReviewersPanel({
const { handleRequestReview, handleRemoveReviewers, requestReviewer } =
createReviewerRequestActions({
submitting,
submittingRef,
setSubmitting,
reviewerInput,
setReviewerInput,
@@ -10,6 +10,7 @@ export function ReviewerPickerRow({
active,
selected,
activeIndex,
disabled,
onHover,
onRequest
}: {
@@ -18,6 +19,7 @@ export function ReviewerPickerRow({
active: boolean
selected: boolean
activeIndex: number
disabled: boolean
onHover: (index: number) => void
onRequest: (reviewer: GitHubAssignableUser) => void
}): React.JSX.Element {
@@ -25,6 +27,7 @@ export function ReviewerPickerRow({
<button
key={`${suggested ? 'suggested' : 'reviewer'}:${reviewer.login}`}
type="button"
disabled={disabled}
aria-label={
selected
? translate(
@@ -38,7 +41,7 @@ export function ReviewerPickerRow({
}
aria-pressed={selected}
className={cn(
'flex min-h-10 w-full items-center gap-2 border-b border-border/70 px-3 py-2 text-left text-[13px] outline-none last:border-b-0 hover:bg-accent/70 focus-visible:bg-accent focus-visible:text-accent-foreground',
'flex min-h-10 w-full items-center gap-2 border-b border-border/70 px-3 py-2 text-left text-[13px] outline-none last:border-b-0 hover:bg-accent/70 focus-visible:bg-accent focus-visible:text-accent-foreground disabled:pointer-events-none disabled:opacity-50',
active && 'bg-accent text-accent-foreground',
selected && 'font-medium'
)}
@@ -132,6 +132,7 @@ export function ReviewerPicker({
active={actionableReviewerRows[activeReviewerIndex]?.login === reviewer.login}
selected={selectedReviewerLogins.has(reviewer.login.toLowerCase())}
activeIndex={index}
disabled={submitting || !canRequestReview}
onHover={onActiveIndexChange}
onRequest={onRequestReviewer}
/>
@@ -150,6 +151,7 @@ export function ReviewerPicker({
active={actionableReviewerRows[activeReviewerIndex]?.login === reviewer.login}
selected={selectedReviewerLogins.has(reviewer.login.toLowerCase())}
activeIndex={suggestedReviewerRows.length + index}
disabled={submitting || !canRequestReview}
onHover={onActiveIndexChange}
onRequest={onRequestReviewer}
/>
@@ -17,7 +17,8 @@ import type { TaskSourceContext } from '../../../../../shared/task-source-contex
import type { GlobalSettings } from '../../../../../shared/global-settings-types'
type ReviewerRequestActionsArgs = {
submitting: boolean
// Why: a ref, not the render-time `submitting` value, so two clicks in one tick can't both pass the guard.
submittingRef: { current: boolean }
setSubmitting: (value: boolean) => void
reviewerInput: string
setReviewerInput: (value: string) => void
@@ -47,7 +48,7 @@ export function createReviewerRequestActions(args: ReviewerRequestActionsArgs):
requestReviewer: (reviewer: GitHubAssignableUser) => Promise<void>
} {
const handleRequestReview = async (requestedLogins?: string[]): Promise<void> => {
if (args.submitting) {
if (args.submittingRef.current) {
return
}
const logins = normalizeGitHubReviewerLogins(
@@ -77,6 +78,7 @@ export function createReviewerRequestActions(args: ReviewerRequestActionsArgs):
)
return
}
args.submittingRef.current = true
args.setSubmitting(true)
try {
const runtimeRepo = getGitHubRuntimeRepoId(args.sourceContext, args.item.repoId)
@@ -146,6 +148,7 @@ export function createReviewerRequestActions(args: ReviewerRequestActionsArgs):
)
}
} finally {
args.submittingRef.current = false
if (args.reviewerPanelMountedRef.current) {
args.setSubmitting(false)
}
@@ -153,7 +156,7 @@ export function createReviewerRequestActions(args: ReviewerRequestActionsArgs):
}
const handleRemoveReviewers = async (reviewersToRemove: string[]): Promise<void> => {
if (args.submitting) {
if (args.submittingRef.current) {
return
}
const selected = new Set(
@@ -175,6 +178,7 @@ export function createReviewerRequestActions(args: ReviewerRequestActionsArgs):
)
return
}
args.submittingRef.current = true
args.setSubmitting(true)
try {
const runtimeRepo = getGitHubRuntimeRepoId(args.sourceContext, args.item.repoId)
@@ -243,6 +247,7 @@ export function createReviewerRequestActions(args: ReviewerRequestActionsArgs):
)
}
} finally {
args.submittingRef.current = false
if (args.reviewerPanelMountedRef.current) {
args.setSubmitting(false)
}
+4 -1
View File
@@ -1692,7 +1692,10 @@
"checkActionRequiredHint": "This check needs a manual action on GitHub (for example, approving the workflow run) before merging is unblocked.",
"6b1d5ee3e4": "No inline details are available for this check.",
"e04c027d98": "Failed to load check details.",
"5df7c41d2a": "Retry"
"5df7c41d2a": "Retry",
"77482513f8": "Close",
"2f5195c6a0": "Reopen",
"4b8ae7303f": "Failed to load diff."
},
"QuickOpen": {
"1dbd3f59ff": "Move",
+4 -1
View File
@@ -1539,7 +1539,10 @@
"dd5d9a4f17": "actualizado {{value0}}",
"71a3c0f9d2": "Iniciar espacio de trabajo",
"filesUnavailable": "Couldn't load changed files.",
"filesRetry": "Retry"
"filesRetry": "Retry",
"77482513f8": "Cerrar",
"2f5195c6a0": "Reabrir",
"4b8ae7303f": "No se pudo cargar la diferencia."
},
"QuickOpen": {
"1dbd3f59ff": "Mover",
+4 -1
View File
@@ -1539,7 +1539,10 @@
"dd5d9a4f17": "{{value0}}に更新",
"71a3c0f9d2": "ワークスペースを開始",
"filesUnavailable": "変更されたファイルを読み込めませんでした。",
"filesRetry": "再試行"
"filesRetry": "再試行",
"77482513f8": "クローズ",
"2f5195c6a0": "再オープン",
"4b8ae7303f": "差分の読み込みに失敗しました。"
},
"QuickOpen": {
"1dbd3f59ff": "移動",
+4 -1
View File
@@ -1539,7 +1539,10 @@
"dd5d9a4f17": "{{value0}}에 업데이트됨",
"71a3c0f9d2": "워크스페이스 시작",
"filesUnavailable": "변경된 파일을 불러오지 못했습니다.",
"filesRetry": "Retry"
"filesRetry": "Retry",
"77482513f8": "닫기",
"2f5195c6a0": "다시 열기",
"4b8ae7303f": "차이를 불러오지 못했습니다."
},
"QuickOpen": {
"1dbd3f59ff": "이동",
+4 -1
View File
@@ -1539,7 +1539,10 @@
"dd5d9a4f17": "{{value0}}更新",
"71a3c0f9d2": "启动工作区",
"filesUnavailable": "无法加载变更的文件。",
"filesRetry": "Retry"
"filesRetry": "Retry",
"77482513f8": "关闭",
"2f5195c6a0": "重新开启",
"4b8ae7303f": "加载差异失败。"
},
"QuickOpen": {
"1dbd3f59ff": "移动",