From 55f98bd240ebf775d5d1b097adefee7048283f10 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sun, 16 Aug 2026 10:18:28 -0700 Subject: [PATCH] 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 --- .../actions/merge-actions.ts | 5 ++- .../pull-request-page/comments/reply-form.tsx | 8 +++++ .../pull-request-page/files/line-comment.ts | 34 ++++++++++++------- .../pull-request-page/files/section-loader.ts | 8 +++-- .../pull-request-page/files/view-restore.ts | 17 ++++++++-- .../pull-request-page/page/surface.tsx | 2 ++ .../pull-request-page/page/viewed-sync.ts | 28 +++++++++------ .../pull-request-page/reviewers/panel.tsx | 3 +- .../reviewers/picker-row.tsx | 5 ++- .../pull-request-page/reviewers/picker.tsx | 2 ++ .../reviewers/request-actions.ts | 11 ++++-- src/renderer/src/i18n/locales/en.json | 5 ++- src/renderer/src/i18n/locales/es.json | 5 ++- src/renderer/src/i18n/locales/ja.json | 5 ++- src/renderer/src/i18n/locales/ko.json | 5 ++- src/renderer/src/i18n/locales/zh.json | 5 ++- 16 files changed, 110 insertions(+), 38 deletions(-) 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 5299375e2d6..abc204fa7e4 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 @@ -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, diff --git a/src/renderer/src/components/pull-request-page/comments/reply-form.tsx b/src/renderer/src/components/pull-request-page/comments/reply-form.tsx index 89b428c5d6f..18879f02665 100644 --- a/src/renderer/src/components/pull-request-page/comments/reply-form.tsx +++ b/src/renderer/src/components/pull-request-page/comments/reply-form.tsx @@ -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) diff --git a/src/renderer/src/components/pull-request-page/files/line-comment.ts b/src/renderer/src/components/pull-request-page/files/line-comment.ts index 9a1a5027ce1..d3e719ae255 100644 --- a/src/renderer/src/components/pull-request-page/files/line-comment.ts +++ b/src/renderer/src/components/pull-request-page/files/line-comment.ts @@ -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> + 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 || diff --git a/src/renderer/src/components/pull-request-page/files/section-loader.ts b/src/renderer/src/components/pull-request-page/files/section-loader.ts index 75f0e23173d..c46a4293a83 100644 --- a/src/renderer/src/components/pull-request-page/files/section-loader.ts +++ b/src/renderer/src/components/pull-request-page/files/section-loader.ts @@ -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) 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 0f1f34e2ad8..bade47ca501 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 @@ -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() diff --git a/src/renderer/src/components/pull-request-page/page/surface.tsx b/src/renderer/src/components/pull-request-page/page/surface.tsx index 8fde51d3f5c..34fe4727462 100644 --- a/src/renderer/src/components/pull-request-page/page/surface.tsx +++ b/src/renderer/src/components/pull-request-page/page/surface.tsx @@ -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, diff --git a/src/renderer/src/components/pull-request-page/page/viewed-sync.ts b/src/renderer/src/components/pull-request-page/page/viewed-sync.ts index f6587de4537..a0810f4a7e8 100644 --- a/src/renderer/src/components/pull-request-page/page/viewed-sync.ts +++ b/src/renderer/src/components/pull-request-page/page/viewed-sync.ts @@ -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) diff --git a/src/renderer/src/components/pull-request-page/reviewers/panel.tsx b/src/renderer/src/components/pull-request-page/reviewers/panel.tsx index b7b322297a4..0f9bbec7b45 100644 --- a/src/renderer/src/components/pull-request-page/reviewers/panel.tsx +++ b/src/renderer/src/components/pull-request-page/reviewers/panel.tsx @@ -66,6 +66,7 @@ export function PRReviewersPanel({ : repoOwnerSettings, [repoOwnerSettings, sourceContext] ) + const submittingRef = useRef(false) const reviewerInputRef = useRef(null) const reviewerInputFocusFrameRef = useRef(null) const reviewerPanelMountedRef = useRef(true) @@ -243,7 +244,7 @@ export function PRReviewersPanel({ const { handleRequestReview, handleRemoveReviewers, requestReviewer } = createReviewerRequestActions({ - submitting, + submittingRef, setSubmitting, reviewerInput, setReviewerInput, diff --git a/src/renderer/src/components/pull-request-page/reviewers/picker-row.tsx b/src/renderer/src/components/pull-request-page/reviewers/picker-row.tsx index a52bbe3ce04..ce17b56642d 100644 --- a/src/renderer/src/components/pull-request-page/reviewers/picker-row.tsx +++ b/src/renderer/src/components/pull-request-page/reviewers/picker-row.tsx @@ -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({