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 abc204fa7e4..54e2728cc9f 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 @@ -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) diff --git a/src/renderer/src/components/pull-request-page/checks/details-list-keys.ts b/src/renderer/src/components/pull-request-page/checks/details-list-keys.ts new file mode 100644 index 00000000000..068d6a9c077 --- /dev/null +++ b/src/renderer/src/components/pull-request-page/checks/details-list-keys.ts @@ -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( + items: readonly T[], + contentKey: (item: T) => string +): { item: T; key: string }[] { + const occurrences = new Map() + 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}` } + }) +} diff --git a/src/renderer/src/components/pull-request-page/checks/details.tsx b/src/renderer/src/components/pull-request-page/checks/details.tsx index 60d182a7689..f9f368e0788 100644 --- a/src/renderer/src/components/pull-request-page/checks/details.tsx +++ b/src/renderer/src/components/pull-request-page/checks/details.tsx @@ -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 (
@@ -115,9 +129,9 @@ export function CheckDetailsPanel({ {translate('auto.components.PullRequestPage.8432d17901', 'Annotations')}
- {details!.annotations.map((annotation, index) => ( + {annotationRows.map(({ item: annotation, key }, index) => (
0 && 'border-t border-border/30' @@ -160,9 +174,9 @@ export function CheckDetailsPanel({ {translate('auto.components.PullRequestPage.7720c9c3f5', 'Jobs')}
- {details!.jobs.map((job, index) => ( + {jobRows.map(({ item: job, key }, index) => (
0 && 'border-t border-border/30')} >
diff --git a/src/renderer/src/components/pull-request-page/files/combined-diff-viewer.tsx b/src/renderer/src/components/pull-request-page/files/combined-diff-viewer.tsx index 8285cca3f94..7e7c2920136 100644 --- a/src/renderer/src/components/pull-request-page/files/combined-diff-viewer.tsx +++ b/src/renderer/src/components/pull-request-page/files/combined-diff-viewer.tsx @@ -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), 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 c46a4293a83..ee69e46e1c3 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 @@ -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(promise: Promise): Promise { + let timeout: ReturnType | null = null + return new Promise((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 loadedIndicesRef: MutableRefObject> @@ -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 } } 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 bade47ca501..2f4bcbc2f8e 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 @@ -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 } diff --git a/src/renderer/src/components/pull-request-page/mentions/options.test.ts b/src/renderer/src/components/pull-request-page/mentions/options.test.ts index 698d92bfd88..579f5e468c9 100644 --- a/src/renderer/src/components/pull-request-page/mentions/options.test.ts +++ b/src/renderer/src/components/pull-request-page/mentions/options.test.ts @@ -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: [] }) 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 34fe4727462..0d4d8a50863 100644 --- a/src/renderer/src/components/pull-request-page/page/surface.tsx +++ b/src/renderer/src/components/pull-request-page/page/surface.tsx @@ -284,7 +284,7 @@ export default function PullRequestPage({ async (path: string, viewed: boolean): Promise => syncPullRequestFileViewed({ canUseDetailsRepoContext, - details, + pullRequestId: details?.pullRequestId, workItem, effectiveRepoId, path, @@ -297,7 +297,7 @@ export default function PullRequestPage({ }), [ canUseDetailsRepoContext, - details, + details?.pullRequestId, detailsCacheKey, effectiveRepoId, projectOrigin, 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 a0810f4a7e8..ed0b0d10f64 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 @@ -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 { 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 }) 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 ce17b56642d..0211da993e4 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 @@ -25,7 +25,6 @@ export function ReviewerPickerRow({ }): React.JSX.Element { return (