From b851a5e13fa56595a50bc449f5a2235e32d77cce Mon Sep 17 00:00:00 2001 From: erish Date: Sun, 23 Aug 2026 14:11:55 +0900 Subject: [PATCH] fix(github): add GHES avatar fallback to PR and task views (#13981) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(pr-page): route remaining user avatars through GitHubUserAvatar On a private-mode GitHub Enterprise instance the stored avatar URL 302s to /login, and the renderer's default Electron session carries no cookie, so the image never loads. #8784 added GitHubUserAvatar for exactly this — it degrades to an initials placeholder via onError — but three call sites in PullRequestPage kept a bare : the reviewer picker, the comment author, and the @ mention suggestions. Each only guarded on avatarUrl being absent, so on GHE the URL is present, the placeholder branch never runs, and a broken image is left on screen. The authorAvatarUrl type comment already documents the intended contract ("falls back to the login URL and finally an initials placeholder"). GitHubUserAvatar was already imported in this file for the PR author, so this makes all five avatars in the page consistent. Note the three switched slots now carry the shared border/bg styling, matching the two that already did. Add a boundary test that fails if any avatar is rendered through a bare again. Fixes #13976 * fix(task-page): route GitHub avatar cells through GitHubUserAvatar too Auditing the rest of the GHE avatar path turned up the same bare in TaskPage: GitHubAssigneeAvatar, GHAssigneesCell and PRReviewCell. Fixing only the PR page would leave half of #13976 in place. GitHubAssigneeAvatar is the clearest case — ReviewChipAvatar directly above it already renders through GitHubUserAvatar, so two adjacent functions disagreed on how a GitHub user avatar is drawn. Its border also moves from border-border/40 to /50, matching the neighbour. Linear member avatars in this file are left alone; they use their own provider path and are out of scope here. Move the regression assertions into the existing repro-8784 file rather than a new boundary test — that file already guards PullRequestPage and TaskPage together, so it is where this belongs. The PR-page check matches the pattern instead of specific field names, so a rename or a newly added avatar slot cannot slip past it; the TaskPage check is scoped per function to avoid catching the Linear cells. * test(github): scope the avatar guard per call site and cover TaskPage names Addresses review feedback on the regression guard. The field-name regex missed aliases and resolver expressions, and the PR-page assertion did not require GitHubUserAvatar in each migrated slot — deleting all three would have passed. Reject any bare within the component scope instead, which is safe now that every assertion is scoped to one function. Drive all six slots from one table so each gets its own named case, and extend the display-name contract to TaskPage, which previously went unchecked. The ConversationTab entry carries displayName: null because PRComment has no display-name field. Reverting the fix now fails 11 cases instead of 3. --- src/renderer/src/components/TaskPage.tsx | 51 ++++++--------- .../repro-8784-ghe-avatar-fallback.test.ts | 64 +++++++++++++++++++ .../conversation/comment-card.tsx | 16 ++--- .../pull-request-page/mentions/textarea.tsx | 14 ++-- .../reviewers/picker-row.tsx | 14 ++-- 5 files changed, 104 insertions(+), 55 deletions(-) diff --git a/src/renderer/src/components/TaskPage.tsx b/src/renderer/src/components/TaskPage.tsx index cf6e884b7b0..b0265713971 100644 --- a/src/renderer/src/components/TaskPage.tsx +++ b/src/renderer/src/components/TaskPage.tsx @@ -1438,25 +1438,14 @@ function ReviewChipAvatar({ } function GitHubAssigneeAvatar({ assignee }: { assignee: GitHubAssignableUser }): React.JSX.Element { - if (assignee.avatarUrl) { - return ( - {assignee.login} - ) - } return ( - - {assignee.login.slice(0, 1).toUpperCase()} - + ) } @@ -1896,13 +1885,12 @@ function GHAssigneesCell({ ) : null} - {user.avatarUrl ? ( - - ) : ( - - {user.login.slice(0, 1).toUpperCase()} - - )} + {user.login} {user.name ? ( @@ -2388,13 +2376,12 @@ function PRReviewCell({ {selected ? : null} - {reviewer.avatarUrl ? ( - - ) : ( - - {reviewer.login.slice(0, 1).toUpperCase()} - - )} + {reviewer.login} diff --git a/src/renderer/src/components/github/repro-8784-ghe-avatar-fallback.test.ts b/src/renderer/src/components/github/repro-8784-ghe-avatar-fallback.test.ts index f30da28d603..533274b2a95 100644 --- a/src/renderer/src/components/github/repro-8784-ghe-avatar-fallback.test.ts +++ b/src/renderer/src/components/github/repro-8784-ghe-avatar-fallback.test.ts @@ -51,4 +51,68 @@ describe('issue #8784 GHE avatar fallback (regression)', () => { // Why: list chip must not hardcode github.com/{login}.png. expect(taskPage).not.toMatch(/github\.com\/\$\{reviewer\.login\}\.png/) }) + + // GHES URLs can exist but fail unauthenticated; target slots need onError fallbacks. + // Scope checks because TaskPage also renders non-GitHub provider avatars. + const GITHUB_AVATAR_SLOTS = [ + { + file: 'pull-request-page/reviewers/picker-row.tsx', + fn: 'ReviewerPickerRow', + login: 'reviewer.login', + displayName: 'reviewer.name' + }, + { + file: 'pull-request-page/conversation/comment-card.tsx', + fn: 'ConversationCommentCard', + login: 'comment.author', + displayName: null + }, + { + file: 'pull-request-page/mentions/textarea.tsx', + fn: 'MentionTextarea', + login: 'option.login', + displayName: 'option.name' + }, + { + file: 'TaskPage.tsx', + fn: 'GitHubAssigneeAvatar', + login: 'assignee.login', + displayName: 'assignee.name' + }, + { file: 'TaskPage.tsx', fn: 'GHAssigneesCell', login: 'user.login', displayName: 'user.name' }, + { + file: 'TaskPage.tsx', + fn: 'PRReviewCell', + login: 'reviewer.login', + displayName: 'reviewer.name' + } + ] as const + + function componentBody(file: string, fn: string): string { + const source = readFileSync(join(__dirname, '..', file), 'utf8') + const start = source.indexOf(`function ${fn}`) + expect(start, `${file}: function ${fn} not found`).toBeGreaterThanOrEqual(0) + const next = source.indexOf('\nfunction ', start + 1) + return source.slice(start, next === -1 ? undefined : next) + } + + it.each(GITHUB_AVATAR_SLOTS)( + 'renders the $fn avatar through GitHubUserAvatar (#13976)', + ({ file, fn, login }) => { + const body = componentBody(file, fn) + + // Reject aliases and resolver expressions as well as direct avatarUrl fields. + expect(body, `${fn} still renders a bare img`).not.toMatch(/]/) + expect(body, `${fn} does not use GitHubUserAvatar`).toContain(' slot.displayName !== null))( + 'passes the $fn display name so initials are not reduced to one letter (#13976)', + ({ file, fn, displayName }) => { + expect(componentBody(file, fn)).toContain(`name={${displayName}}`) + } + ) }) 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 66ed253a6c3..598a23ad6ca 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 @@ -12,6 +12,7 @@ import { import { formatRelativeTime } from '@/components/github/work-item-state-presentation' import { CommentCodeContext } from '@/components/github/CommentCodeContext' import { CommentReactions } from '@/components/github/CommentReactions' +import { GitHubUserAvatar } from '@/components/github/github-user-avatar' import { translate } from '@/i18n/i18n' import type { GitHubOwnerRepo, GitHubPRFile } from '../../../../../shared/github/pull-request-types' import type { PRComment } from '../../../../../shared/github/comment-types' @@ -62,15 +63,12 @@ export function ConversationCommentCard({ )} >
- {comment.authorAvatarUrl ? ( - {comment.author} - ) : ( -
- )} + - {option.avatarUrl ? ( - - ) : ( -
- {option.login.slice(0, 1).toUpperCase()} -
- )} + @{option.login} {option.name && ( 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 a8b347765f2..5e24fffdbbb 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 @@ -2,6 +2,7 @@ import React from 'react' import { Check } from 'lucide-react' import { cn } from '@/lib/utils' import { translate } from '@/i18n/i18n' +import { GitHubUserAvatar } from '@/components/github/github-user-avatar' import type { GitHubAssignableUser } from '../../../../../shared/github/pull-request-types' export function ReviewerPickerRow({ @@ -59,13 +60,12 @@ export function ReviewerPickerRow({ {selected ? : null} - {reviewer.avatarUrl ? ( - - ) : ( - - {reviewer.login.slice(0, 1).toUpperCase()} - - )} + {reviewer.login}