mirror of
https://github.com/stablyai/orca.git
synced 2026-10-03 08:02:12 +00:00
fix(github): add GHES avatar fallback to PR and task views (#13981)
* 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 <img>: 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 <img> 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 <img> 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 <img> 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 <img> 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.
This commit is contained in:
@@ -1438,25 +1438,14 @@ function ReviewChipAvatar({
|
||||
}
|
||||
|
||||
function GitHubAssigneeAvatar({ assignee }: { assignee: GitHubAssignableUser }): React.JSX.Element {
|
||||
if (assignee.avatarUrl) {
|
||||
return (
|
||||
<img
|
||||
src={assignee.avatarUrl}
|
||||
alt={assignee.login}
|
||||
loading="lazy"
|
||||
decoding="async"
|
||||
title={assignee.name ? `${assignee.name} (${assignee.login})` : assignee.login}
|
||||
className="size-5 rounded-full border border-border/40 bg-muted object-cover"
|
||||
/>
|
||||
)
|
||||
}
|
||||
return (
|
||||
<span
|
||||
title={assignee.login}
|
||||
className="inline-flex size-5 items-center justify-center rounded-full border border-border/40 bg-muted text-[10px] font-medium text-muted-foreground"
|
||||
>
|
||||
{assignee.login.slice(0, 1).toUpperCase()}
|
||||
</span>
|
||||
<GitHubUserAvatar
|
||||
login={assignee.login}
|
||||
name={assignee.name}
|
||||
avatarUrl={assignee.avatarUrl}
|
||||
title={assignee.name ? `${assignee.name} (${assignee.login})` : assignee.login}
|
||||
className="size-5"
|
||||
/>
|
||||
)
|
||||
}
|
||||
|
||||
@@ -1896,13 +1885,12 @@ function GHAssigneesCell({
|
||||
<Check className="size-3" />
|
||||
) : null}
|
||||
</span>
|
||||
{user.avatarUrl ? (
|
||||
<img src={user.avatarUrl} alt="" className="size-5 shrink-0 rounded-full" />
|
||||
) : (
|
||||
<span className="flex size-5 shrink-0 items-center justify-center rounded-full bg-muted text-[10px] font-medium text-muted-foreground">
|
||||
{user.login.slice(0, 1).toUpperCase()}
|
||||
</span>
|
||||
)}
|
||||
<GitHubUserAvatar
|
||||
login={user.login}
|
||||
name={user.name}
|
||||
avatarUrl={user.avatarUrl}
|
||||
className="size-5"
|
||||
/>
|
||||
<span className="min-w-0 flex-1">
|
||||
<span className="block truncate">{user.login}</span>
|
||||
{user.name ? (
|
||||
@@ -2388,13 +2376,12 @@ function PRReviewCell({
|
||||
<span className="flex size-4 shrink-0 items-center justify-center text-foreground">
|
||||
{selected ? <Check className="size-3.5" /> : null}
|
||||
</span>
|
||||
{reviewer.avatarUrl ? (
|
||||
<img src={reviewer.avatarUrl} alt="" className="size-5 shrink-0 rounded-full" />
|
||||
) : (
|
||||
<span className="flex size-5 shrink-0 items-center justify-center rounded-full bg-muted text-[10px] font-medium text-muted-foreground">
|
||||
{reviewer.login.slice(0, 1).toUpperCase()}
|
||||
</span>
|
||||
)}
|
||||
<GitHubUserAvatar
|
||||
login={reviewer.login}
|
||||
name={reviewer.name}
|
||||
avatarUrl={reviewer.avatarUrl}
|
||||
className="size-5"
|
||||
/>
|
||||
<span className="min-w-0 flex-1">
|
||||
<span className="block truncate">
|
||||
<span className="font-semibold text-foreground">{reviewer.login}</span>
|
||||
|
||||
@@ -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(/<img[\s>]/)
|
||||
expect(body, `${fn} does not use GitHubUserAvatar`).toContain('<GitHubUserAvatar')
|
||||
expect(body, `${fn} does not pass its login`).toContain(`login={${login}}`)
|
||||
}
|
||||
)
|
||||
|
||||
// Preserve two-letter initials when the fallback renders.
|
||||
it.each(GITHUB_AVATAR_SLOTS.filter((slot) => 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}}`)
|
||||
}
|
||||
)
|
||||
})
|
||||
|
||||
@@ -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({
|
||||
)}
|
||||
>
|
||||
<div className="flex min-w-0 items-center gap-2 border-b border-border/40 px-3 py-2">
|
||||
{comment.authorAvatarUrl ? (
|
||||
<img
|
||||
src={comment.authorAvatarUrl}
|
||||
alt={comment.author}
|
||||
className="size-5 shrink-0 rounded-full"
|
||||
/>
|
||||
) : (
|
||||
<div className="size-5 shrink-0 rounded-full bg-muted" />
|
||||
)}
|
||||
<GitHubUserAvatar
|
||||
login={comment.author}
|
||||
avatarUrl={comment.authorAvatarUrl}
|
||||
title={comment.author}
|
||||
className="size-5"
|
||||
/>
|
||||
<span
|
||||
className={cn(
|
||||
'min-w-0 truncate text-[13px] font-semibold',
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
import React, { useCallback, useId, useMemo, useState } from 'react'
|
||||
import { cn } from '@/lib/utils'
|
||||
import { filterGitHubMentionOptions } from '@/components/github/github-mention-option-filter'
|
||||
import { GitHubUserAvatar } from '@/components/github/github-user-avatar'
|
||||
import type { MentionOption, MentionQuery } from '../page-types'
|
||||
import { findMentionQuery } from './query'
|
||||
|
||||
@@ -86,13 +87,12 @@ export function MentionTextarea({
|
||||
index === activeIndex && 'bg-accent text-accent-foreground'
|
||||
)}
|
||||
>
|
||||
{option.avatarUrl ? (
|
||||
<img src={option.avatarUrl} alt="" className="size-5 shrink-0 rounded-full" />
|
||||
) : (
|
||||
<div className="flex size-5 shrink-0 items-center justify-center rounded-full bg-muted text-[10px] font-medium text-muted-foreground">
|
||||
{option.login.slice(0, 1).toUpperCase()}
|
||||
</div>
|
||||
)}
|
||||
<GitHubUserAvatar
|
||||
login={option.login}
|
||||
name={option.name}
|
||||
avatarUrl={option.avatarUrl}
|
||||
className="size-5"
|
||||
/>
|
||||
<span className="flex min-w-0 flex-1 items-baseline gap-1.5">
|
||||
<span className="shrink-0 font-medium">@{option.login}</span>
|
||||
{option.name && (
|
||||
|
||||
@@ -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({
|
||||
<span className="flex size-4 shrink-0 items-center justify-center text-foreground">
|
||||
{selected ? <Check className="size-3.5" /> : null}
|
||||
</span>
|
||||
{reviewer.avatarUrl ? (
|
||||
<img src={reviewer.avatarUrl} alt="" className="size-5 shrink-0 rounded-full" />
|
||||
) : (
|
||||
<span className="flex size-5 shrink-0 items-center justify-center rounded-full bg-muted text-[10px] font-medium text-muted-foreground">
|
||||
{reviewer.login.slice(0, 1).toUpperCase()}
|
||||
</span>
|
||||
)}
|
||||
<GitHubUserAvatar
|
||||
login={reviewer.login}
|
||||
name={reviewer.name}
|
||||
avatarUrl={reviewer.avatarUrl}
|
||||
className="size-5"
|
||||
/>
|
||||
<span className="min-w-0 flex-1">
|
||||
<span className="block truncate">
|
||||
<span className="font-semibold text-foreground">{reviewer.login}</span>
|
||||
|
||||
Reference in New Issue
Block a user