Improve PR page stability: add timeout and stable list keys

- Add 45s timeout for diff loading to prevent indefinite hangs - Fix React list key generation for annotations/jobs using content-based keys with occurrence tracking - Refactor scroll position caching to properly handle mid-restore teardown - Replace interpolated error messages with full locale-specific strings for close/reopen actions
2026-10-23 00:03:52 +00:00 · 2026-08-16 19:21:35 -07:00
parent 55f98bd240
commit 4e9e7b435f
15 changed files with 141 additions and 58 deletions
@@ -86,12 +86,19 @@ export async function changePullRequestState(args: {
    args.onMutated()
  } catch (err) {
    args.applyStatePatch(previousState)
+    // Why: full sentences per state — interpolating the localized verb into a template mangles other locales.
    toast.error(
      err instanceof Error
        ? err.message
-        : translate('auto.components.PullRequestPage.b8c6cbb8c4', 'Failed to {{value0}} PR', {
-            value0: label.toLowerCase()
-          })
+        : args.nextState === 'closed'
+          ? translate(
+              'auto.components.PullRequestPage.closePullRequestFailed',
+              'Failed to close pull request'
+            )
+          : translate(
+              'auto.components.PullRequestPage.reopenPullRequestFailed',
+              'Failed to reopen pull request'
+            )
    )
  } finally {
    args.setStatePending(false)
@@ -0,0 +1,18 @@
+/**
+ * Check annotations and jobs carry no stable id of their own: GitHub workflow-level annotations
+ * repeat identical content (`path`/`startLine` null, same message), and GitLab jobs may report a
+ * null id. Keying on content alone therefore collides; keying on the array index alone reshuffles
+ * rows on every list update. Pairing the content key with its occurrence count gives both.
+ */
+export function assignUniqueListKeys<T>(
+  items: readonly T[],
+  contentKey: (item: T) => string
+): { item: T; key: string }[] {
+  const occurrences = new Map<string, number>()
+  return items.map((item) => {
+    const base = contentKey(item)
+    const seen = occurrences.get(base) ?? 0
+    occurrences.set(base, seen + 1)
+    return { item, key: seen === 0 ? base : `${base}#${seen}` }
+  })
+}
@@ -12,6 +12,7 @@ import {
 import type { CheckDetailsLoadState } from '@/components/github-checks-tab-state'
 import { translate } from '@/i18n/i18n'
 import type { PRCheckDetail } from '../../../../../shared/github/check-types'
+import { assignUniqueListKeys } from './details-list-keys'

 export function CheckDetailsPanel({
  check,
@@ -32,8 +33,21 @@ export function CheckDetailsPanel({
    conclusion: (details?.conclusion as PRCheckDetail['conclusion'] | undefined) ?? check.conclusion
  }
  const hasOutput = Boolean(details?.title || details?.summary || details?.text)
-  const hasAnnotations = (details?.annotations.length ?? 0) > 0
-  const hasJobs = (details?.jobs.length ?? 0) > 0
+  const annotationRows = assignUniqueListKeys(details?.annotations ?? [], (annotation) =>
+    [
+      annotation.path ?? 'annotation',
+      annotation.startLine ?? '',
+      annotation.endLine ?? '',
+      annotation.annotationLevel ?? '',
+      annotation.title ?? '',
+      annotation.message
+    ].join('\0')
+  )
+  const jobRows = assignUniqueListKeys(details?.jobs ?? [], (job) =>
+    job.id === null || job.id === undefined ? `name:${job.name}` : `id:${job.id}`
+  )
+  const hasAnnotations = annotationRows.length > 0
+  const hasJobs = jobRows.length > 0

  return (
    <div className="mx-2 mb-2 mt-1 min-w-0 rounded-md border border-border/50 bg-muted/20 px-3 py-2">
@@ -115,9 +129,9 @@ export function CheckDetailsPanel({
                {translate('auto.components.PullRequestPage.8432d17901', 'Annotations')}
              </div>
              <div className="flex flex-col">
-                {details!.annotations.map((annotation, index) => (
+                {annotationRows.map(({ item: annotation, key }, index) => (
                  <div
-                    key={`${annotation.path ?? 'annotation'}-${annotation.startLine ?? ''}-${annotation.endLine ?? ''}-${annotation.annotationLevel ?? ''}-${annotation.title ?? ''}-${annotation.message}-${annotation.rawDetails ?? ''}`}
+                    key={key}
                    className={cn(
                      'min-w-0 px-2.5 py-2 text-[12px]',
                      index > 0 && 'border-t border-border/30'
@@ -160,9 +174,9 @@ export function CheckDetailsPanel({
                {translate('auto.components.PullRequestPage.7720c9c3f5', 'Jobs')}
              </div>
              <div className="flex flex-col">
-                {details!.jobs.map((job, index) => (
+                {jobRows.map(({ item: job, key }, index) => (
                  <div
-                    key={job.id ?? `${job.name}-${job.startedAt ?? ''}`}
+                    key={key}
                    className={cn('min-w-0 px-2.5 py-2', index > 0 && 'border-t border-border/30')}
                  >
                    <div className="flex min-w-0 items-center gap-2">
@@ -1,3 +1,4 @@
+/* oxlint-disable react-doctor/no-adjust-state-on-prop-change -- Why: switching PRs re-seeds the whole diff view (sections, heights, scroll target, cached loaded set) from the persisted cache, which cannot be derived during render. */
 import React, { useCallback, useEffect, useLayoutEffect, useMemo, useRef, useState } from 'react'
 import { useVirtualizer } from '@tanstack/react-virtual'
 import type { editor as monacoEditor } from 'monaco-editor'
@@ -136,8 +137,8 @@ export function PRFilesCombinedDiffViewer({
    loadedIndicesRef.current.clear()
    loadingIndicesRef.current.clear()
    pendingRestoreScrollTopRef.current = prFilesDiffScrollTopCache.get(viewStateKey) ?? null
-    setSectionHeights(entries.length > 0 ? {} : {})
-    setActiveTreeSectionKey(entries.length > 0 ? null : null)
+    setSectionHeights({})
+    setActiveTreeSectionKey(null)
    setSections(
      entries.map((entry) => ({
        key: getPRFileSectionKey(entry.path),
@@ -19,6 +19,34 @@ import type {
 import type { TaskSourceContext } from '../../../../../shared/task-source-context'
 import { loadPRFileContents } from '../cache/file-content'

+// Why: the local IPC path carries no timeout, so a wedged host would leave the section spinning
+// forever with no error and therefore no retry button. Above the 30s remote RPC timeout so a
+// remote failure still surfaces its own message.
+const PR_FILE_DIFF_LOAD_TIMEOUT_MS = 45_000
+
+function rejectAfterLoadTimeout<T>(promise: Promise<T>): Promise<T> {
+  let timeout: ReturnType<typeof setTimeout> | null = null
+  return new Promise<T>((resolve, reject) => {
+    timeout = setTimeout(
+      () =>
+        reject(
+          new Error(
+            translate(
+              'auto.components.PullRequestPage.diffLoadTimedOut',
+              'Timed out loading this diff.'
+            )
+          )
+        ),
+      PR_FILE_DIFF_LOAD_TIMEOUT_MS
+    )
+    promise.then(resolve, reject)
+  }).finally(() => {
+    if (timeout) {
+      clearTimeout(timeout)
+    }
+  })
+}
+
 export function usePRFileSectionLoader(args: {
  sectionsRef: MutableRefObject<DiffSection[]>
  loadedIndicesRef: MutableRefObject<Set<number>>
@@ -103,16 +131,18 @@ export function usePRFileSectionLoader(args: {
            )
          }
        }
-        const contents = await loadPRFileContents({
-          repoPath,
-          repoId,
-          sourceContext,
-          prNumber,
-          prRepo,
-          file,
-          headSha,
-          baseSha
-        })
+        const contents = await rejectAfterLoadTimeout(
+          loadPRFileContents({
+            repoPath,
+            repoId,
+            sourceContext,
+            prNumber,
+            prRepo,
+            file,
+            headSha,
+            baseSha
+          })
+        )
        return { result: getPRFileDiffResult(contents), resultContents: contents }
      }

@@ -55,26 +55,32 @@ export function usePRFilesDiffViewPersistence(args: {
      return
    }

-    const updateCachedScrollPosition = (): void => {
+    const writeCachedScrollPosition = (scrollTop: number): void => {
+      const existing = prFilesDiffViewStateCache.get(args.viewStateKey)
+      setWithLRU(prFilesDiffScrollTopCache, args.viewStateKey, scrollTop)
+      if (!existing || existing.entrySignature !== args.entrySignature) {
+        return
+      }
+      setWithLRU(prFilesDiffViewStateCache, args.viewStateKey, { ...existing, scrollTop })
+    }
+
+    const cacheScrollPositionOnScroll = (): 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) {
-        return
-      }
-      setWithLRU(prFilesDiffViewStateCache, args.viewStateKey, {
-        ...existing,
-        scrollTop: container.scrollTop
-      })
+      writeCachedScrollPosition(container.scrollTop)
    }

-    container.addEventListener('scroll', updateCachedScrollPosition)
+    const cacheScrollPositionOnTeardown = (): void => {
+      // Why: tearing down mid-restore must persist the pending target, not the clamped intermediate.
+      writeCachedScrollPosition(args.pendingRestoreScrollTopRef.current ?? container.scrollTop)
+    }
+
+    container.addEventListener('scroll', cacheScrollPositionOnScroll)
    return () => {
-      updateCachedScrollPosition()
-      container.removeEventListener('scroll', updateCachedScrollPosition)
+      cacheScrollPositionOnTeardown()
+      container.removeEventListener('scroll', cacheScrollPositionOnScroll)
    }
  }, [
    args.entrySignature,
@@ -115,7 +121,8 @@ export function usePRFilesDiffViewPersistence(args: {
        frameId = window.requestAnimationFrame(restoreScrollPosition)
        return
      }
-      // Why: give up rather than block scroll caching forever when the content never grows enough.
+      // Why: cache what we actually reached; an unreachable target would replay this fight every visit.
+      setWithLRU(prFilesDiffScrollTopCache, args.viewStateKey, liveContainer.scrollTop)
      args.pendingRestoreScrollTopRef.current = null
    }

@@ -37,11 +37,11 @@ describe('buildMentionOptions', () => {
    expect(options[2]?.name).toBe('Helper')
  })

-  it('skips ghost and fills missing avatar/name on a later duplicate login', () => {
+  it('skips ghost and fills missing avatar/name on a later case-insensitive duplicate login', () => {
    const options = buildMentionOptions({
      item,
-      comments: [comment('ghost'), comment('octocat', 'https://avatars/octocat')],
-      participants: [user('octocat', { name: 'Mona' })],
+      comments: [comment('ghost'), comment('Octocat', 'https://avatars/octocat')],
+      participants: [user('OCTOCAT', { name: 'Mona' })],
      assignableUsers: []
    })

@@ -284,7 +284,7 @@ export default function PullRequestPage({
    async (path: string, viewed: boolean): Promise<boolean> =>
      syncPullRequestFileViewed({
        canUseDetailsRepoContext,
-        details,
+        pullRequestId: details?.pullRequestId,
        workItem,
        effectiveRepoId,
        path,
@@ -297,7 +297,7 @@ export default function PullRequestPage({
      }),
    [
      canUseDetailsRepoContext,
-      details,
+      details?.pullRequestId,
      detailsCacheKey,
      effectiveRepoId,
      projectOrigin,
@@ -3,17 +3,14 @@ import { setPRFileViewedForRepo } from '@/components/github/github-work-item-com
 import { resolvePullRequestRepo } from '@/components/github/github-work-item-identity'
 import { translate } from '@/i18n/i18n'
 import type { GitHubPRFileViewedState } from '../../../../../shared/github/pull-request-types'
-import type {
-  GitHubWorkItem,
-  GitHubWorkItemDetails
-} from '../../../../../shared/github/work-item-types'
+import type { GitHubWorkItem } from '../../../../../shared/github/work-item-types'
 import type { TaskSourceContext } from '../../../../../shared/task-source-context'
 import type { PullRequestPageProjectOrigin } from '../page-types'
 import { patchCachedPRFileViewedState } from '../cache/work-item-details'

 export async function syncPullRequestFileViewed(args: {
  canUseDetailsRepoContext: boolean
-  details: GitHubWorkItemDetails | null
+  pullRequestId: string | undefined
  workItem: GitHubWorkItem | null
  effectiveRepoId: string | null
  path: string
@@ -26,7 +23,7 @@ export async function syncPullRequestFileViewed(args: {
 }): Promise<boolean> {
  if (
    !args.canUseDetailsRepoContext ||
-    !args.details?.pullRequestId ||
+    !args.pullRequestId ||
    !args.workItem ||
    args.workItem.type !== 'pr'
  ) {
@@ -62,7 +59,7 @@ export async function syncPullRequestFileViewed(args: {
      sourceContext: args.sourceContext,
      prNumber: args.workItem.number,
      prRepo: resolvePullRequestRepo(args.workItem, args.projectOrigin),
-      pullRequestId: args.details.pullRequestId,
+      pullRequestId: args.pullRequestId,
      path: args.path,
      viewed: args.viewed
    })
@@ -25,7 +25,6 @@ export function ReviewerPickerRow({
 }): React.JSX.Element {
  return (
    <button
-      key={`${suggested ? 'suggested' : 'reviewer'}:${reviewer.login}`}
      type="button"
      disabled={disabled}
      aria-label={
@@ -1594,7 +1594,6 @@
        "c57873d721": "Pull request merged",
        "a63b3c159c": "This will update the pull request on GitHub.",
        "eec3706a6a": "{{value0}} PR #{{value1}}?",
-        "b8c6cbb8c4": "Failed to {{value0}} PR",
        "710e47aa06": "Pull request reopened",
        "7aa3b5f706": "Pull request closed",
        "3d77438c92": "This will reopen the pull request on GitHub.",
@@ -1695,7 +1694,10 @@
        "5df7c41d2a": "Retry",
        "77482513f8": "Close",
        "2f5195c6a0": "Reopen",
-        "4b8ae7303f": "Failed to load diff."
+        "4b8ae7303f": "Failed to load diff.",
+        "closePullRequestFailed": "Failed to close pull request",
+        "reopenPullRequestFailed": "Failed to reopen pull request",
+        "diffLoadTimedOut": "Timed out loading this diff."
      },
      "QuickOpen": {
        "1dbd3f59ff": "Move",
@@ -1440,7 +1440,6 @@
        "c57873d721": "Pull request mergeado",
        "a63b3c159c": "Esto actualizará el pull request en GitHub.",
        "eec3706a6a": "{{value0}} PR #{{value1}}?",
-        "b8c6cbb8c4": "No se pudo {{value0}} PR",
        "710e47aa06": "Pull request reabierto",
        "7aa3b5f706": "Pull request cerrado",
        "3d77438c92": "Esto reabrirá el pull request en GitHub.",
@@ -1542,7 +1541,10 @@
        "filesRetry": "Retry",
        "77482513f8": "Cerrar",
        "2f5195c6a0": "Reabrir",
-        "4b8ae7303f": "No se pudo cargar la diferencia."
+        "4b8ae7303f": "No se pudo cargar la diferencia.",
+        "closePullRequestFailed": "No se pudo cerrar el pull request",
+        "reopenPullRequestFailed": "No se pudo reabrir el pull request",
+        "diffLoadTimedOut": "Se agotó el tiempo de espera al cargar esta diferencia."
      },
      "QuickOpen": {
        "1dbd3f59ff": "Mover",
@@ -1440,7 +1440,6 @@
        "c57873d721": "PR がマージされました",
        "a63b3c159c": "これにより、GitHub 上の PR が更新されます。",
        "eec3706a6a": "{{value0}} PR #{{value1}}?",
-        "b8c6cbb8c4": "{{value0}} PR に失敗しました",
        "710e47aa06": "PR が再オープンされました",
        "7aa3b5f706": "PR がクローズされました",
        "3d77438c92": "これにより、GitHub で PR が再開されます。",
@@ -1542,7 +1541,10 @@
        "filesRetry": "再試行",
        "77482513f8": "クローズ",
        "2f5195c6a0": "再オープン",
-        "4b8ae7303f": "差分の読み込みに失敗しました。"
+        "4b8ae7303f": "差分の読み込みに失敗しました。",
+        "closePullRequestFailed": "PR のクローズに失敗しました",
+        "reopenPullRequestFailed": "PR の再オープンに失敗しました",
+        "diffLoadTimedOut": "この差分の読み込みがタイムアウトしました。"
      },
      "QuickOpen": {
        "1dbd3f59ff": "移動",
@@ -1440,7 +1440,6 @@
        "c57873d721": "PR이 병합되었습니다",
        "a63b3c159c": "그러면 GitHub의 PR이 업데이트됩니다.",
        "eec3706a6a": "{{value0}} PR #{{value1}}?",
-        "b8c6cbb8c4": "PR {{value0}}에 실패했습니다.",
        "710e47aa06": "PR이 다시 열렸습니다",
        "7aa3b5f706": "PR이 종료되었습니다.",
        "3d77438c92": "그러면 GitHub에서 PR이 다시 열립니다.",
@@ -1542,7 +1541,10 @@
        "filesRetry": "Retry",
        "77482513f8": "닫기",
        "2f5195c6a0": "다시 열기",
-        "4b8ae7303f": "차이를 불러오지 못했습니다."
+        "4b8ae7303f": "차이를 불러오지 못했습니다.",
+        "closePullRequestFailed": "PR을 닫지 못했습니다.",
+        "reopenPullRequestFailed": "PR을 다시 열지 못했습니다.",
+        "diffLoadTimedOut": "이 diff를 불러오는 데 시간이 초과되었습니다."
      },
      "QuickOpen": {
        "1dbd3f59ff": "이동",
@@ -1440,7 +1440,6 @@
        "c57873d721": "拉取请求已合并",
        "a63b3c159c": "这将更新 GitHub 上的PR。",
        "eec3706a6a": "{{value0}} PR #{{value1}}？",
-        "b8c6cbb8c4": "{{value0}} PR 失败",
        "710e47aa06": "拉取请求已重新打开",
        "7aa3b5f706": "PR已关闭",
        "3d77438c92": "这将在 GitHub 上重新打开PR。",
@@ -1542,7 +1541,10 @@
        "filesRetry": "Retry",
        "77482513f8": "关闭",
        "2f5195c6a0": "重新开启",
-        "4b8ae7303f": "加载差异失败。"
+        "4b8ae7303f": "加载差异失败。",
+        "closePullRequestFailed": "关闭PR失败",
+        "reopenPullRequestFailed": "重新打开PR失败",
+        "diffLoadTimedOut": "加载此差异超时。"
      },
      "QuickOpen": {
        "1dbd3f59ff": "移动",