Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cmd/e2e-server/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -1129,6 +1129,7 @@ func buildAppState(
caps: platform.Capabilities{
ReadRepositories: true,
ReadIssues: true,
AssigneeMutation: true,
},
repos: []platform.Repository{{
Ref: platform.RepoRef{
Expand Down
42 changes: 42 additions & 0 deletions frontend/src/lib/components/detail/UserPicker.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,48 @@ describe("UserPicker", () => {
expect(screen.getAllByRole("menuitemcheckbox")).toHaveLength(2);
});

it("renders synced profile images when an avatar URL is available", () => {
render(UserPicker, {
props: {
title: "Edit reviewers",
candidates: ["alice", "manual-user"],
selected: [],
avatarUrlForUser: (username) => (username === "alice" ? "https://github.example/alice.png?size=40" : ""),
ontoggle: vi.fn(),
onclose: vi.fn(),
},
});

const aliceRow = screen.getByRole("menuitemcheckbox", { name: /alice/i });
const aliceAvatar = aliceRow.querySelector("img.user-picker__avatar");
expect(aliceAvatar?.getAttribute("src")).toBe("https://github.example/alice.png?size=40");
expect(aliceAvatar?.getAttribute("alt")).toBe("");

expect(screen.getByRole("menuitemcheckbox", { name: /manual-user/i }).textContent).toContain("M");
});

it("falls back to initials when a profile image fails to load", async () => {
render(UserPicker, {
props: {
title: "Edit reviewers",
candidates: ["roborev-ci"],
selected: [],
avatarUrlForUser: () => "https://github.example/roborev-ci.png?size=40",
ontoggle: vi.fn(),
onclose: vi.fn(),
},
});

const row = screen.getByRole("menuitemcheckbox", { name: /roborev-ci/i });
const avatar = row.querySelector("img.user-picker__avatar");
expect(avatar).toBeTruthy();

await fireEvent.error(avatar as HTMLImageElement);

expect(row.querySelector("img.user-picker__avatar")).toBeNull();
expect(row.textContent).toContain("R");
});

it("notifies query changes so callers can fetch matching candidates", async () => {
const onQuery = vi.fn();
render(UserPicker, {
Expand Down
31 changes: 30 additions & 1 deletion frontend/tests/e2e-full/assignees-reviewers.spec.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { expect, test } from "@playwright/test";
import { startIsolatedE2EServer, type IsolatedE2EServer } from "./support/e2eServer";
import { startIsolatedE2EServer, startIsolatedE2EServerWithOptions, type IsolatedE2EServer } from "./support/e2eServer";

test.describe("assignee and reviewer editing", () => {
test("pull detail edits assignees and persists across reload", async ({ page }) => {
Expand All @@ -15,6 +15,9 @@ test.describe("assignee and reviewer editing", () => {
await page.getByRole("button", { name: "Edit assignees" }).click();
await expect(page.getByRole("dialog", { name: "Edit assignees" })).toBeVisible();
await expect(page.getByRole("menuitemcheckbox", { name: /alice/i })).toHaveAttribute("aria-checked", "true");
await expect(
page.getByRole("menuitemcheckbox", { name: /alice/i }).locator("img.user-picker__avatar"),
).toHaveAttribute("src", "https://github.com/alice.png?size=40");
await expect(page.getByRole("menuitemcheckbox", { name: /bob/i })).toHaveAttribute("aria-checked", "false");

// The picker is a compact dropdown: start-aligned under its
Expand Down Expand Up @@ -60,6 +63,9 @@ test.describe("assignee and reviewer editing", () => {
await page.getByRole("button", { name: "Edit reviewers" }).click();
await expect(page.getByRole("dialog", { name: "Edit reviewers" })).toBeVisible();
await expect(page.getByRole("menuitemcheckbox", { name: /carol/i })).toHaveAttribute("aria-checked", "true");
await expect(
page.getByRole("menuitemcheckbox", { name: /carol/i }).locator("img.user-picker__avatar"),
).toHaveAttribute("src", "https://github.com/carol.png?size=40");

// Removing the only requested reviewer issues a PUT with an empty set.
const removeResponse = page.waitForResponse(
Expand Down Expand Up @@ -95,6 +101,9 @@ test.describe("assignee and reviewer editing", () => {
await expect(page.locator(".pull-detail")).toBeVisible();
await page.getByRole("button", { name: "Edit assignees" }).click();
await expect(page.getByRole("dialog", { name: "Edit assignees" })).toBeVisible();
await expect(
page.getByRole("menuitemcheckbox", { name: /alice/i }).locator("img.user-picker__avatar"),
).toHaveAttribute("src", "https://github.com/alice.png?size=40");

// Typing requeries the autocomplete endpoint with the filter so
// candidates beyond the first page stay reachable.
Expand Down Expand Up @@ -273,4 +282,24 @@ test.describe("assignee and reviewer editing", () => {
await isolatedServer?.stop();
}
});

test("non-GitHub issue picker falls back to initials without provider avatar data", async ({ page }) => {
let isolatedServer: IsolatedE2EServer | null = null;
try {
isolatedServer = await startIsolatedE2EServerWithOptions({ providerCollision: true });
const baseURL = isolatedServer.info.base_url;

await page.goto(`${baseURL}/host/github.com/issues/gitea/acme/widgets/901`);
await expect(page.locator(".issue-detail")).toBeVisible();
await page.getByRole("button", { name: "Edit assignees" }).click();
await expect(page.getByRole("dialog", { name: "Edit assignees" })).toBeVisible();

const aliceRow = page.getByRole("menuitemcheckbox", { name: /alice/i });
await expect(aliceRow).toHaveAttribute("aria-checked", "false");
await expect(aliceRow.locator("img.user-picker__avatar")).toHaveCount(0);
await expect(aliceRow.locator(".user-picker__avatar")).toHaveText("A");
} finally {
await isolatedServer?.stop();
}
});
});
10 changes: 10 additions & 0 deletions internal/platform/gitlab/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ type clientOptions struct {
baseURL string
foregroundTimeout time.Duration
rateTracker *ratelimit.RateTracker
disableRetries bool
}

type Client struct {
Expand Down Expand Up @@ -84,6 +85,12 @@ func WithRateTracker(rateTracker *ratelimit.RateTracker) ClientOption {
}
}

func WithoutRetriesForTesting() ClientOption {
return func(opts *clientOptions) {
opts.disableRetries = true
}
}

func NewClient(host string, source tokenauth.Source, options ...ClientOption) (*Client, error) {
opts := clientOptions{
baseURL: "https://" + strings.TrimRight(host, "/") + "/api/v4",
Expand All @@ -94,6 +101,9 @@ func NewClient(host string, source tokenauth.Source, options ...ClientOption) (*
}

clientOptions := []gitlab.ClientOptionFunc{gitlab.WithBaseURL(opts.baseURL)}
if opts.disableRetries {
clientOptions = append(clientOptions, gitlab.WithoutRetries())
}
baseTransport := http.DefaultTransport
if opts.rateTracker != nil {
baseTransport = &rateTrackingTransport{
Expand Down
26 changes: 25 additions & 1 deletion internal/platform/gitlab/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,27 @@ func TestClientLooksUpProjectByRawPathAndUsesNumericIDAfterLookup(t *testing.T)
}, paths)
}

func TestClientTestHelperDisablesRetries(t *testing.T) {
assert := assert.New(t)
require := require.New(t)
requests := 0
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
requests++
http.Error(w, "rate limited", http.StatusTooManyRequests)
}))
defer server.Close()

client := newTestClient(t, server.URL)
_, err := client.GetRepository(context.Background(), platform.RepoRef{
Platform: platform.KindGitLab,
Host: "gitlab.example.com",
RepoPath: "group/project",
})

require.Error(err)
assert.Equal(1, requests)
}

func TestClientListOpenMergeRequestsPopulatesForkHeadRepoCloneURL(t *testing.T) {
assert := assert.New(t)
require := require.New(t)
Expand Down Expand Up @@ -717,7 +738,10 @@ func TestSelfHostedBaseURLConstruction(t *testing.T) {

func newTestClient(t *testing.T, serverURL string, opts ...ClientOption) *Client {
t.Helper()
allOpts := append([]ClientOption{WithBaseURLForTesting(serverURL + "/api/v4")}, opts...)
allOpts := append([]ClientOption{
WithBaseURLForTesting(serverURL + "/api/v4"),
WithoutRetriesForTesting(),
}, opts...)
client, err := NewClient("gitlab.example.com", testTokenSource("token"), allOpts...)
require.NoError(t, err)
return client
Expand Down
14 changes: 13 additions & 1 deletion packages/ui/src/components/detail/IssueDetail.svelte
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<script lang="ts">
import { tick, untrack } from "svelte";
import { providerItemPath, providerRepoPath, providerRouteParams } from "../../api/provider-routes.js";
import { canonicalProvider, providerItemPath, providerRepoPath, providerRouteParams } from "../../api/provider-routes.js";
import type { Label, ProviderCapabilities } from "../../api/types.js";
import {
getStores, getClient, getActions,
Expand Down Expand Up @@ -350,6 +350,17 @@
return data?.users ?? [];
}

function userAvatarURL(username: string): string {
if (canonicalProvider(provider) !== "github") return "";
const login = encodeURIComponent(username.trim());
const host = issues.getIssueDetail()?.repo?.platform_host
?? issues.getIssueDetail()?.platform_host
?? platformHost
?? "";
if (login === "" || host === "") return "";
return `https://${host}/${login}.png?size=40`;
}

function onDocumentMousedown(e: MouseEvent): void {
if (!labelPickerOpen) return;
const target = e.target as Node;
Expand Down Expand Up @@ -904,6 +915,7 @@
disabled={staleIssue || assigneeGate.unavailable}
disabledReason={assigneeGate.unavailable ? assigneeGate.reason : undefined}
loadCandidates={loadUserCandidates}
avatarUrlForUser={userAvatarURL}
onchange={(next) => issues.setIssueAssignees(owner, name, number, next)}
>
{#snippet icon()}
Expand Down
14 changes: 14 additions & 0 deletions packages/ui/src/components/detail/PullDetail.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
import CopyItemNumber from "./CopyItemNumber.svelte";
import { DiffSummaryFilesResult } from "./diff-summary.js";
import {
canonicalProvider,
providerItemPath,
providerRepoPath,
providerRouteParams,
Expand Down Expand Up @@ -1047,6 +1048,17 @@
return data?.users ?? [];
}

function userAvatarURL(username: string): string {
if (canonicalProvider(provider) !== "github") return "";
const login = encodeURIComponent(username.trim());
const host = detailStore.getDetail()?.repo?.platform_host
?? detailStore.getDetail()?.platform_host
?? platformHost
?? "";
if (login === "" || host === "") return "";
return `https://${host}/${login}.png?size=40`;
}

function onActionMenuKeydown(e: KeyboardEvent): void {
if (actionMenuOpen && e.key === "Escape") {
actionMenuOpen = false;
Expand Down Expand Up @@ -1633,6 +1645,7 @@
disabled={stalePR || assigneeGate.unavailable}
disabledReason={assigneeGate.unavailable ? assigneeGate.reason : undefined}
loadCandidates={loadUserCandidates}
avatarUrlForUser={userAvatarURL}
onchange={(next) => detailStore.setPullAssignees(owner, name, number, next)}
>
{#snippet icon()}
Expand All @@ -1647,6 +1660,7 @@
disabledReason={reviewerGate.unavailable ? reviewerGate.reason : undefined}
tooltipNote="User review requests only; team requests are not shown"
loadCandidates={loadUserCandidates}
avatarUrlForUser={userAvatarURL}
onchange={(next) => detailStore.setPullReviewers(owner, name, number, next)}
>
{#snippet icon()}
Expand Down
3 changes: 3 additions & 0 deletions packages/ui/src/components/detail/UserListEditor.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
/// with "" when the picker opens and again as the user types, so
/// candidates beyond the first page stay reachable by searching.
loadCandidates: (query: string) => Promise<string[]>;
avatarUrlForUser?: ((username: string) => string) | undefined;
onchange: (next: string[]) => Promise<unknown>;
icon?: Snippet;
}
Expand All @@ -41,6 +42,7 @@
disabledReason = undefined,
tooltipNote = undefined,
loadCandidates,
avatarUrlForUser = undefined,
onchange,
icon = undefined,
}: Props = $props();
Expand Down Expand Up @@ -267,6 +269,7 @@
{pendingUser}
error={mutationError ?? candidatesError}
{autofocusFilter}
{avatarUrlForUser}
onquery={onPickerQuery}
ontoggle={toggleUser}
onclear={clearUsers}
Expand Down
29 changes: 28 additions & 1 deletion packages/ui/src/components/detail/UserPicker.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
pendingUser?: string | null;
error?: string | null;
autofocusFilter?: boolean;
avatarUrlForUser?: ((username: string) => string) | undefined;
/// The query the current candidates were fetched for. When set,
/// the exact-username entry row is withheld until the candidate
/// list reflects the typed query, so a stale list cannot offer a
Expand All @@ -33,6 +34,7 @@
pendingUser = null,
error = null,
autofocusFilter = false,
avatarUrlForUser = undefined,
candidatesQuery = undefined,
onquery = undefined,
ontoggle,
Expand All @@ -42,6 +44,7 @@

let query = $state("");
let filterInput: HTMLInputElement | undefined = $state();
let failedAvatarKeys = $state<string[]>([]);

onMount(() => {
if (autofocusFilter) filterInput?.focus();
Expand Down Expand Up @@ -81,6 +84,16 @@
if (pendingUser !== null || selectedNames.size === 0) return;
void onclear?.();
}

function avatarKey(username: string): string {
return username.toLowerCase();
}

function markAvatarFailed(username: string): void {
const key = avatarKey(username);
if (failedAvatarKeys.includes(key)) return;
failedAvatarKeys = [...failedAvatarKeys, key];
}
</script>

<div class="user-picker" role="dialog" aria-label={title}>
Expand Down Expand Up @@ -132,6 +145,8 @@
<div class="user-picker__list" role="menu" aria-label="Users">
{#each listedUsers as username (username.toLowerCase())}
{@const isSelected = selectedNames.has(username.toLowerCase())}
{@const avatarURL = avatarUrlForUser?.(username) ?? ""}
{@const showAvatarImage = avatarURL !== "" && !failedAvatarKeys.includes(avatarKey(username))}
<button
type="button"
class={["user-picker__row", { "user-picker__row--selected": isSelected }]}
Expand All @@ -140,7 +155,18 @@
disabled={pendingUser !== null}
onclick={() => ontoggle(username)}
>
<span class="user-picker__avatar" aria-hidden="true">{username.slice(0, 1).toUpperCase()}</span>
{#if showAvatarImage}
<img
class="user-picker__avatar"
src={avatarURL}
alt=""
loading="lazy"
aria-hidden="true"
onerror={() => markAvatarFailed(username)}
/>
{:else}
<span class="user-picker__avatar" aria-hidden="true">{username.slice(0, 1).toUpperCase()}</span>
{/if}
<span class="user-picker__name">{username}</span>
<span class="user-picker__status">
{#if pendingUser === username}
Expand Down Expand Up @@ -338,6 +364,7 @@
color: var(--text-secondary);
font-size: var(--font-size-xs);
font-weight: 700;
object-fit: cover;
}

.user-picker__name {
Expand Down
Loading