From a0b2734c2045ae5a6a5febc9d136f4ce09825384 Mon Sep 17 00:00:00 2001 From: Pavel Kroupa <63880977+Tabonx@users.noreply.github.com> Date: Sat, 24 Oct 2026 08:09:54 +0200 Subject: [PATCH 1/6] fix(source-control): Azure DevOps pull requests show profile pictures Azure DevOps identity images only load for a signed-in browser session, so every pull request avatar fell back to an initial. The Azure DevOps definition now claims identity image URLs through `mediaUrl`, and its driver's `fetchMedia` reads them through `az devops invoke`, which uses the same credential as every other `az devops` command (AZURE_DEVOPS_EXT_PAT, a PAT from `az devops login`, or the `az login` session). Avatars are cached per identity. Web, desktop, and mobile ask for any avatar a host keeps behind a credential as source control media, the same path pull request body media takes. `cwd` on source control media is now optional, and the server no longer signs it. Nothing has read it since GitHub media stopped going through `gh`, and an avatar in a pull request list has no repository behind it. Clients still send it for body media, because older servers require it. --- .../PullRequestDiscussionScreen.tsx | 6 +- .../pull-requests/PullRequestListRow.tsx | 6 +- .../pull-requests/PullRequestPresentation.tsx | 68 ++++++++-- .../PullRequestReviewersScreen.tsx | 2 +- .../pull-requests/PullRequestRouteScreen.tsx | 12 +- .../pull-requests/PullRequestStatusRows.tsx | 14 ++- .../pull-requests/PullRequestTimeline.tsx | 25 +++- .../threads/LinkedPullRequestList.tsx | 10 +- .../threads/ThreadPullRequestsSheet.tsx | 1 + .../features/threads/git/GitOverviewSheet.tsx | 1 + apps/server/src/assets/AssetAccess.test.ts | 23 +++- apps/server/src/assets/AssetAccess.ts | 4 - .../src/assets/sourceControlMediaResponse.ts | 2 +- .../pullRequest/PullRequestDetailPanel.tsx | 8 +- .../pullRequest/PullRequestGhosts.tsx | 10 +- .../pullRequest/PullRequestLinkPreview.tsx | 6 +- .../pullRequest/PullRequestListFilters.tsx | 2 +- .../pullRequest/PullRequestListRow.tsx | 5 +- .../PullRequestReviewAnnotation.tsx | 2 +- .../pullRequest/PullRequestReviewerPicker.tsx | 6 +- .../components/pullRequest/PullRequestRow.tsx | 1 + .../pullRequest/PullRequestSummaryTab.tsx | 18 ++- .../pullRequest/PullRequestTimelineTab.tsx | 18 ++- .../pullRequest/ThreadPullRequestsPanel.tsx | 1 + .../pullRequest/pullRequestList.logic.ts | 8 +- .../pullRequest/pullRequestPresentation.tsx | 84 ++++++++++--- packages/contracts/src/assets.ts | 9 +- .../src/server/AzureDevOpsCli.test.ts | 110 ++++++++++++++++ .../src/server/AzureDevOpsCli.ts | 118 ++++++++++++++++++ .../src/server/azureDevOpsAvatarFetch.test.ts | 54 ++++++++ .../src/server/azureDevOpsAvatarFetch.ts | 40 ++++++ .../src/server/driver.ts | 2 + .../src/shared/azureDevOpsAvatar.test.ts | 50 ++++++++ .../src/shared/azureDevOpsAvatar.ts | 50 ++++++++ .../src/shared/definition.ts | 2 + 35 files changed, 721 insertions(+), 57 deletions(-) create mode 100644 packages/source-control-azure-devops/src/server/azureDevOpsAvatarFetch.test.ts create mode 100644 packages/source-control-azure-devops/src/server/azureDevOpsAvatarFetch.ts create mode 100644 packages/source-control-azure-devops/src/shared/azureDevOpsAvatar.test.ts create mode 100644 packages/source-control-azure-devops/src/shared/azureDevOpsAvatar.ts diff --git a/apps/mobile/src/features/pull-requests/PullRequestDiscussionScreen.tsx b/apps/mobile/src/features/pull-requests/PullRequestDiscussionScreen.tsx index ecd811f495408..dfcd605b4ff77 100644 --- a/apps/mobile/src/features/pull-requests/PullRequestDiscussionScreen.tsx +++ b/apps/mobile/src/features/pull-requests/PullRequestDiscussionScreen.tsx @@ -201,7 +201,11 @@ function Discussion({ params }: { params: PullRequestParams & { readonly discuss {comments.map((comment) => ( - + {comment.author?.login ?? "Unknown author"} diff --git a/apps/mobile/src/features/pull-requests/PullRequestListRow.tsx b/apps/mobile/src/features/pull-requests/PullRequestListRow.tsx index f8f7a5f9c5b15..1c1cf77f8175a 100644 --- a/apps/mobile/src/features/pull-requests/PullRequestListRow.tsx +++ b/apps/mobile/src/features/pull-requests/PullRequestListRow.tsx @@ -109,7 +109,11 @@ export const PullRequestListRow = memo( )} > - + {item.repository} #{item.number} diff --git a/apps/mobile/src/features/pull-requests/PullRequestPresentation.tsx b/apps/mobile/src/features/pull-requests/PullRequestPresentation.tsx index a050013249be2..2eba49abf6461 100644 --- a/apps/mobile/src/features/pull-requests/PullRequestPresentation.tsx +++ b/apps/mobile/src/features/pull-requests/PullRequestPresentation.tsx @@ -1,6 +1,13 @@ import { AppActivityIndicator } from "../../components/ui/AppActivityIndicator"; -import type { ChangeRequestTerminology } from "@t3tools/client-runtime/source-control-clients"; -import type { PullRequestActor } from "@t3tools/contracts"; +import { + sourceControlClients, + type ChangeRequestTerminology, +} from "@t3tools/client-runtime/source-control-clients"; +import { + SOURCE_CONTROL_MEDIA_ASSET, + type EnvironmentId, + type PullRequestActor, +} from "@t3tools/contracts"; import IconAlertTriangle from "@tabler/icons-react-native/IconAlertTriangle"; import { Image } from "expo-image"; import { View } from "react-native"; @@ -9,6 +16,7 @@ import { useResolveClassNames } from "uniwind"; import { AppText as Text } from "../../components/ui/AppText"; import { cn } from "../../lib/cn"; import { EmptyState } from "../../components/ui/EmptyState"; +import { useAssetUrlState } from "../../state/assets"; export function PullRequestReadStatus(props: { readonly pending: boolean; @@ -41,14 +49,60 @@ export function PullRequestConflictBadge(props: { readonly surfaceClassName: str export function PullRequestAvatar(props: { readonly actor: PullRequestActor | null; + /** Needed for a host, such as Azure DevOps, whose avatars only the environment's credential reads. */ + readonly environmentId?: EnvironmentId | null | undefined; readonly size?: number; }) { const size = props.size ?? 24; - return props.actor?.avatarUrl ? ( + const login = props.actor?.login ?? null; + const avatarUrl = props.actor?.avatarUrl ?? null; + const hostMedia = avatarUrl === null ? null : sourceControlClients.mediaUrl(avatarUrl); + if (hostMedia !== null) { + return ( + + ); + } + return ; +} + +function HostAvatar(props: { + readonly login: string | null; + readonly fetchUrl: string; + readonly environmentId: EnvironmentId | null; + readonly size: number; +}) { + const asset = useAssetUrlState(props.environmentId, { + _tag: SOURCE_CONTROL_MEDIA_ASSET, + url: props.fetchUrl, + }); + return ( + + ); +} + +function AvatarImage(props: { + readonly login: string | null; + readonly uri: string | null; + readonly cacheKey?: string; + readonly size: number; +}) { + const { size } = props; + return props.uri ? ( @@ -58,7 +112,7 @@ export function PullRequestAvatar(props: { style={{ width: size, height: size }} > - {props.actor?.login.slice(0, 1).toUpperCase() ?? "?"} + {props.login?.slice(0, 1).toUpperCase() ?? "?"} ); diff --git a/apps/mobile/src/features/pull-requests/PullRequestReviewersScreen.tsx b/apps/mobile/src/features/pull-requests/PullRequestReviewersScreen.tsx index 1fa5cd83b874f..c82a8408a5495 100644 --- a/apps/mobile/src/features/pull-requests/PullRequestReviewersScreen.tsx +++ b/apps/mobile/src/features/pull-requests/PullRequestReviewersScreen.tsx @@ -210,7 +210,7 @@ export function PullRequestReviewersScreen({ route }: StaticScreenProps void update(item)} className="min-h-16 flex-row items-center gap-3 border-b border-border-subtle px-4 py-3 active:opacity-70" > - + {item.name || item.login} diff --git a/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx b/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx index 9c7f5ead34fb4..db9aaa2f1a422 100644 --- a/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx +++ b/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx @@ -46,7 +46,11 @@ import { useFocusEffect, useNavigation, type StaticScreenProps } from "@react-na import type { NativeStackNavigationProp } from "@react-navigation/native-stack"; import { RegistryContext, useAtomRefresh, useAtomValue } from "@effect/atom-react"; import { pullRequestReviewKey } from "./pullRequestReviewDrafts"; -import { type PullRequestDetail, type PullRequestListEntry } from "@t3tools/contracts"; +import { + EnvironmentId, + type PullRequestDetail, + type PullRequestListEntry, +} from "@t3tools/contracts"; import { PullRequestOverviewLoading } from "./PullRequestOverviewLoading"; import { copyPullRequestLink, sharePullRequestLink } from "./pullRequestLink"; import { pullRequestStatus } from "./pullRequestStatus"; @@ -204,7 +208,11 @@ function PullRequestIdentity(props: { {pr.title} - + {pr.author?.login ?? "Unknown author"} diff --git a/apps/mobile/src/features/pull-requests/PullRequestStatusRows.tsx b/apps/mobile/src/features/pull-requests/PullRequestStatusRows.tsx index 185d555cc776b..cc309d56a6b55 100644 --- a/apps/mobile/src/features/pull-requests/PullRequestStatusRows.tsx +++ b/apps/mobile/src/features/pull-requests/PullRequestStatusRows.tsx @@ -1,7 +1,7 @@ import { usePullRequestConnected } from "./usePullRequestConnected"; import { useAtomValue } from "@effect/atom-react"; import { useNavigation } from "@react-navigation/native"; -import type { PullRequestAction, PullRequestDetail } from "@t3tools/contracts"; +import { EnvironmentId, type PullRequestAction, type PullRequestDetail } from "@t3tools/contracts"; import { capitalizeTerm, sourceControlClients, @@ -300,7 +300,11 @@ export function PullRequestReviewersRow(props: RowProps) { : "accent-icon-muted" } /> - + {review.author?.login ?? "Unknown reviewer"} @@ -318,7 +322,11 @@ export function PullRequestReviewersRow(props: RowProps) { className="min-h-14 flex-row items-center gap-3 px-4 py-2" > - + {reviewer.login} diff --git a/apps/mobile/src/features/pull-requests/PullRequestTimeline.tsx b/apps/mobile/src/features/pull-requests/PullRequestTimeline.tsx index 424f8cdff1b31..daf5cf5fc515b 100644 --- a/apps/mobile/src/features/pull-requests/PullRequestTimeline.tsx +++ b/apps/mobile/src/features/pull-requests/PullRequestTimeline.tsx @@ -2,7 +2,11 @@ import { PullRequestEntryRow } from "./PullRequestEntryRow"; import Svg, { Defs, LinearGradient, Rect, Stop } from "react-native-svg"; import { useAppearancePreferences } from "../settings/appearance/AppearancePreferencesProvider"; import IconGitCommit from "@tabler/icons-react-native/IconGitCommit"; -import type { PullRequestActivity, PullRequestComment } from "@t3tools/contracts"; +import { + EnvironmentId, + type PullRequestActivity, + type PullRequestComment, +} from "@t3tools/contracts"; import { StackActions, type NavigationProp } from "@react-navigation/native"; import { useMemo, useState, type ComponentProps, type ReactNode } from "react"; import { Pressable, StyleSheet, View } from "react-native"; @@ -88,7 +92,11 @@ function TimelineComment({ tone={approved ? "success" : changes ? "danger" : undefined} > - + {comment.author?.login ?? "Unknown author"} @@ -199,6 +207,7 @@ export function pullRequestTimelineRows( navigation: Pick, "navigate" | "dispatch">, actions: CommentActions, ) { + const environmentId = EnvironmentId.make(params.environmentId); const discussionIds = new Set( activity.reviewThreads.flatMap((thread) => thread.comments.map((comment) => comment.id)), ); @@ -216,7 +225,11 @@ export function pullRequestTimelineRows( content: ( - + {thread.comments[0]?.author?.login ?? "Unknown author"} @@ -269,7 +282,11 @@ export function pullRequestTimelineRows( } className="flex-row items-center gap-2 active:opacity-70" > - + {commit.messageHeadline} diff --git a/apps/mobile/src/features/threads/LinkedPullRequestList.tsx b/apps/mobile/src/features/threads/LinkedPullRequestList.tsx index 3fa45480f12be..1f2064f59ad6d 100644 --- a/apps/mobile/src/features/threads/LinkedPullRequestList.tsx +++ b/apps/mobile/src/features/threads/LinkedPullRequestList.tsx @@ -1,4 +1,5 @@ import type { EnvironmentThreadShell } from "@t3tools/client-runtime/state/shell"; +import type { EnvironmentId } from "@t3tools/contracts"; import { capitalizeTerm, sourceControlClients, @@ -58,6 +59,7 @@ export function linkedPullRequestKey(line: PullRequestListLine) { export const LinkedPullRequestRow = memo(function LinkedPullRequestRow(props: { readonly line: PullRequestListLine; + readonly environmentId: EnvironmentId | null; readonly separator: boolean; readonly surfaceClassName: string; readonly openPullRequest: (url: string) => Promise; @@ -114,7 +116,13 @@ export const LinkedPullRequestRow = memo(function LinkedPullRequestRow(props: { )} > - {snapshot?.author ? : null} + {snapshot?.author ? ( + + ) : null} {link.repository} diff --git a/apps/mobile/src/features/threads/ThreadPullRequestsSheet.tsx b/apps/mobile/src/features/threads/ThreadPullRequestsSheet.tsx index 6eb94d3c232b8..011f17496cf29 100644 --- a/apps/mobile/src/features/threads/ThreadPullRequestsSheet.tsx +++ b/apps/mobile/src/features/threads/ThreadPullRequestsSheet.tsx @@ -58,6 +58,7 @@ export function ThreadPullRequestsSheet( renderItem={({ item, index }) => ( { describe("readMcpAppHtml", () => { it.effect("accepts snapshots at the stored HTML byte limit", () => @@ -1429,7 +1431,6 @@ describe("AssetAccess", () => { expect(asset).toEqual({ kind: "source-control-media", url: media.fetchUrl, - cwd: "/repo", expiresAt: issued.expiresAt, }); if (asset?.kind !== "source-control-media") return; @@ -1446,6 +1447,26 @@ describe("AssetAccess", () => { }).pipe(Effect.provide(Layer.merge(layerTest, layerDrivers)), Effect.scoped); }); + it.effect("signs host media a client asks for without a repository", () => + Effect.gen(function* () { + // A pull request list draws avatars with no repository at hand. + const { resource } = yield* decodeCreateUrlInput({ + resource: { + _tag: "github-media", + url: "https://dev.azure.com/acme/_api/_common/identityImage?id=15F0488E-B980-68CA-9D71-8A387C166D09", + }, + }); + const issued = yield* issueAssetUrl({ resource }); + const suffix = issued.relativeUrl.slice(`${ASSET_ROUTE_PREFIX}/`.length); + const separator = suffix.indexOf("/"); + expect(yield* resolveAsset(suffix.slice(0, separator), suffix.slice(separator + 1))).toEqual({ + kind: "source-control-media", + url: "https://dev.azure.com/acme/_api/_common/identityImage?id=15f0488e-b980-68ca-9d71-8a387c166d09", + expiresAt: issued.expiresAt, + }); + }).pipe(Effect.provide(layerTest)), + ); + it.effect("refuses media no host keeps behind a credential, without echoing the URL", () => Effect.gen(function* () { const encodeAssetAccessError = Schema.encodeEffect(Schema.fromJsonString(AssetAccessError)); diff --git a/apps/server/src/assets/AssetAccess.ts b/apps/server/src/assets/AssetAccess.ts index 63c0f74f1772c..b741b7ece7dc7 100644 --- a/apps/server/src/assets/AssetAccess.ts +++ b/apps/server/src/assets/AssetAccess.ts @@ -174,7 +174,6 @@ const AssetClaimsSchema = Schema.Union([ kind: Schema.Literal(SOURCE_CONTROL_MEDIA_ASSET), /** A host's `fetchUrl`, narrowed at mint time; the signature is what keeps it there. */ url: Schema.String, - cwd: Schema.String, expiresAt: Schema.Number, }), ]); @@ -202,7 +201,6 @@ export type ResolvedAsset = | { readonly kind: "source-control-media"; readonly url: string; - readonly cwd: string; readonly expiresAt: number; }; @@ -840,7 +838,6 @@ export const issueAssetUrl = Effect.fn("AssetAccess.issueAssetUrl")(function* (i version: 1, kind: SOURCE_CONTROL_MEDIA_ASSET, url: media.fetchUrl, - cwd: input.resource.cwd, expiresAt, }; fileName = media.fileName; @@ -956,7 +953,6 @@ export const resolveAsset = Effect.fn("AssetAccess.resolveAsset")(function* ( return { kind: "source-control-media", url: claims.url, - cwd: claims.cwd, expiresAt: claims.expiresAt, } satisfies ResolvedAsset; } diff --git a/apps/server/src/assets/sourceControlMediaResponse.ts b/apps/server/src/assets/sourceControlMediaResponse.ts index 96e96f32a178a..f6abe00edb5fc 100644 --- a/apps/server/src/assets/sourceControlMediaResponse.ts +++ b/apps/server/src/assets/sourceControlMediaResponse.ts @@ -25,7 +25,7 @@ const SVG_CONTENT_SECURITY_POLICY = "default-src 'none'; style-src 'unsafe-inlin export const sourceControlMediaResponse = Effect.fn( "SourceControlMediaResponse.sourceControlMediaResponse", )(function* ( - asset: { readonly url: string; readonly cwd: string; readonly expiresAt: number }, + asset: { readonly url: string; readonly expiresAt: number }, requestHeaders: Record, ) { const remainingSeconds = Math.floor((asset.expiresAt - (yield* Clock.currentTimeMillis)) / 1000); diff --git a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx index 9c3faaf31677b..16e066f352485 100644 --- a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx @@ -1230,6 +1230,7 @@ export function PullRequestDetailPanel({ return ( } author={ - + } updated={ diff --git a/apps/web/src/components/pullRequest/PullRequestGhosts.tsx b/apps/web/src/components/pullRequest/PullRequestGhosts.tsx index b9192068294b6..c1eb36cbfffad 100644 --- a/apps/web/src/components/pullRequest/PullRequestGhosts.tsx +++ b/apps/web/src/components/pullRequest/PullRequestGhosts.tsx @@ -1,4 +1,4 @@ -import type { PullRequestListEntry, PullRequestSummary } from "@t3tools/contracts"; +import type { EnvironmentId, PullRequestListEntry, PullRequestSummary } from "@t3tools/contracts"; import { ArrowLeftIcon, ChevronRightIcon, @@ -82,6 +82,7 @@ export function PullRequestListGhost({ rows = 7, caption }: { rows?: number; cap export function PullRequestDetailGhost({ seed: entry, + environmentId, summary, actions, checkoutCommand, @@ -93,6 +94,7 @@ export function PullRequestDetailGhost({ onCheckoutError, }: { seed?: PullRequestListEntry | null; + environmentId?: EnvironmentId; summary?: PullRequestSummary | null; actions?: ReactNode; checkoutCommand?: string | null; @@ -205,7 +207,11 @@ export function PullRequestDetailGhost({ } author={ seed ? ( - + ) : ( diff --git a/apps/web/src/components/pullRequest/PullRequestLinkPreview.tsx b/apps/web/src/components/pullRequest/PullRequestLinkPreview.tsx index f5ed4ea6553bf..79835df457c01 100644 --- a/apps/web/src/components/pullRequest/PullRequestLinkPreview.tsx +++ b/apps/web/src/components/pullRequest/PullRequestLinkPreview.tsx @@ -135,7 +135,11 @@ export function PullRequestLinkPreview({ {preview.title}

- + {authorLabel} ยท diff --git a/apps/web/src/components/pullRequest/PullRequestListFilters.tsx b/apps/web/src/components/pullRequest/PullRequestListFilters.tsx index 912e4c6d7ac4d..272afba31e1ee 100644 --- a/apps/web/src/components/pullRequest/PullRequestListFilters.tsx +++ b/apps/web/src/components/pullRequest/PullRequestListFilters.tsx @@ -311,7 +311,7 @@ function PullRequestAuthorChoices({ {visible.map((option) => ( - + {option.actor.login} {option.mergedCount > 0 ? ( diff --git a/apps/web/src/components/pullRequest/PullRequestListRow.tsx b/apps/web/src/components/pullRequest/PullRequestListRow.tsx index feec84fac0110..8062d5a9b238e 100644 --- a/apps/web/src/components/pullRequest/PullRequestListRow.tsx +++ b/apps/web/src/components/pullRequest/PullRequestListRow.tsx @@ -1,4 +1,5 @@ import type { + EnvironmentId, PullRequestActor, PullRequestMergeability, PullRequestState, @@ -105,10 +106,12 @@ export function PullRequestRowLines({ export function PullRequestRowAuthor({ actor, + environmentId, className, labelClassName, }: { actor: PullRequestActor | null; + environmentId: EnvironmentId; className?: string; labelClassName?: string; }) { @@ -118,7 +121,7 @@ export function PullRequestRowAuthor({ } > - + {login} diff --git a/apps/web/src/components/pullRequest/PullRequestReviewAnnotation.tsx b/apps/web/src/components/pullRequest/PullRequestReviewAnnotation.tsx index 57b50934b7806..9b2596edf2d9c 100644 --- a/apps/web/src/components/pullRequest/PullRequestReviewAnnotation.tsx +++ b/apps/web/src/components/pullRequest/PullRequestReviewAnnotation.tsx @@ -264,7 +264,7 @@ export function ReviewThreadCard({ {comments.map((comment) => (
- + diff --git a/apps/web/src/components/pullRequest/PullRequestReviewerPicker.tsx b/apps/web/src/components/pullRequest/PullRequestReviewerPicker.tsx index b03f4f56758b5..6d0fd0f637a66 100644 --- a/apps/web/src/components/pullRequest/PullRequestReviewerPicker.tsx +++ b/apps/web/src/components/pullRequest/PullRequestReviewerPicker.tsx @@ -107,7 +107,11 @@ export function PullRequestReviewerPicker({ > {(candidate) => ( <> - + {candidate.kind === "team" ? ( team ) : null} diff --git a/apps/web/src/components/pullRequest/PullRequestRow.tsx b/apps/web/src/components/pullRequest/PullRequestRow.tsx index 3b4d1f84c01d9..76bd754e64120 100644 --- a/apps/web/src/components/pullRequest/PullRequestRow.tsx +++ b/apps/web/src/components/pullRequest/PullRequestRow.tsx @@ -224,6 +224,7 @@ function PullRequestRowImpl({ ) : null} diff --git a/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx b/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx index 39aa50593b3e2..d12c1cfdd7d2f 100644 --- a/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx @@ -88,9 +88,11 @@ function reviewerKey(login: string): string { function CommentIdentity({ comment, detail, + environmentId, }: { comment: PullRequestComment; detail: PullRequestDetailView; + environmentId: EnvironmentId; }) { const actor = comment.author; const profileUrl = actor @@ -98,7 +100,12 @@ function CommentIdentity({ : null; return (
- +
- + {replyCount > 0 ? ( {replyCount} {replyCount === 1 ? "reply" : "replies"} @@ -573,7 +584,7 @@ export function PullRequestSummaryTab({ >
- + {outcome ? ( ) : comment.reviewState ? ( @@ -685,6 +696,7 @@ export function PullRequestSummaryTab({ > diff --git a/apps/web/src/components/pullRequest/PullRequestTimelineTab.tsx b/apps/web/src/components/pullRequest/PullRequestTimelineTab.tsx index b522094e90c98..719a85f78495a 100644 --- a/apps/web/src/components/pullRequest/PullRequestTimelineTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestTimelineTab.tsx @@ -129,11 +129,13 @@ function IconMarker({ icon, className }: { icon: ReactNode; className?: string | function ActorTimelineMarker({ actors, + environmentId, className, fallback, muted = false, }: { actors: ReadonlyArray; + environmentId: EnvironmentId; className?: string | undefined; fallback: ReactNode; muted?: boolean; @@ -145,6 +147,7 @@ function ActorTimelineMarker({ } muted={!open} @@ -389,6 +393,7 @@ function ReviewThreadEvent({
} muted={!expanded} @@ -465,9 +470,11 @@ function ReviewThreadEvent({ function CommitEvent({ event, + environmentId, onOpen, }: { event: PullRequestTimelineEvent; + environmentId: EnvironmentId; onOpen: (oid: string) => void; }) { return ( @@ -479,6 +486,7 @@ function CommitEvent({ > } />
@@ -558,6 +566,7 @@ function ReviewVerdictEvent({
} /> @@ -699,7 +708,14 @@ export function PullRequestTimelineTab({ ); } if (event.kind === "commit") { - return ; + return ( + + ); } const outcome = pullRequestReviewOutcome(event.reviewState); if (outcome !== null) { diff --git a/apps/web/src/components/pullRequest/ThreadPullRequestsPanel.tsx b/apps/web/src/components/pullRequest/ThreadPullRequestsPanel.tsx index 1070ed8448d75..b597550b6795b 100644 --- a/apps/web/src/components/pullRequest/ThreadPullRequestsPanel.tsx +++ b/apps/web/src/components/pullRequest/ThreadPullRequestsPanel.tsx @@ -253,6 +253,7 @@ function LinkRowImpl({ {snapshot?.author ? ( diff --git a/apps/web/src/components/pullRequest/pullRequestList.logic.ts b/apps/web/src/components/pullRequest/pullRequestList.logic.ts index 2db366c0e2918..a2130364846e9 100644 --- a/apps/web/src/components/pullRequest/pullRequestList.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestList.logic.ts @@ -47,6 +47,11 @@ export interface PullRequestGroup, + entries: ReadonlyArray, state: PullRequestListState, ) { const authors = new Map(); @@ -93,6 +98,7 @@ export function collectPullRequestListFacets( const held = authors.get(key); authors.set(key, { actor: held?.actor ?? entry.author, + environmentId: held?.environmentId ?? entry.environmentId, count: (held?.count ?? 0) + Number(inState), mergedCount: (held?.mergedCount ?? 0) + Number(entry.state === "merged"), }); diff --git a/apps/web/src/components/pullRequest/pullRequestPresentation.tsx b/apps/web/src/components/pullRequest/pullRequestPresentation.tsx index 1d077e337172c..86f0f97f3dea7 100644 --- a/apps/web/src/components/pullRequest/pullRequestPresentation.tsx +++ b/apps/web/src/components/pullRequest/pullRequestPresentation.tsx @@ -1,15 +1,18 @@ import { Spinner } from "~/components/ui/spinner"; -import type { - PullRequestActor, - PullRequestCheck, - PullRequestCheckStatus, - PullRequestChecksState, - PullRequestLabel, - PullRequestMergeability, - PullRequestReviewDecision, - PullRequestState, +import { + SOURCE_CONTROL_MEDIA_ASSET, + type EnvironmentId, + type PullRequestActor, + type PullRequestCheck, + type PullRequestCheckStatus, + type PullRequestChecksState, + type PullRequestLabel, + type PullRequestMergeability, + type PullRequestReviewDecision, + type PullRequestState, } from "@t3tools/contracts"; import { formatDiffCount } from "@t3tools/client-runtime/diff-count"; +import { sourceControlClients } from "@t3tools/client-runtime/source-control-clients"; import { CircleCheckIcon, CircleDashedIcon, @@ -22,6 +25,7 @@ import { } from "lucide-react"; import { Children, type CSSProperties, isValidElement, type ReactNode, useState } from "react"; +import { useAssetUrlState } from "~/assets/assetUrls"; import { cn } from "~/lib/utils"; import { Badge } from "../ui/badge"; @@ -367,15 +371,65 @@ export function PullRequestReviewOutcomeBadge({ export function PullRequestActorAvatar({ actor, + environmentId, className, }: { actor: PullRequestActor | null; + /** Needed for a host, such as Azure DevOps, whose avatars only the environment's credential reads. */ + environmentId?: EnvironmentId | null | undefined; className?: string; }) { const login = actor?.login ?? "ghost"; const avatarUrl = actor?.avatarUrl ?? null; - const [failedAvatarUrl, setFailedAvatarUrl] = useState(null); - return avatarUrl === null || failedAvatarUrl === avatarUrl ? ( + const hostMedia = avatarUrl === null ? null : sourceControlClients.mediaUrl(avatarUrl); + if (hostMedia !== null) { + return ( + + ); + } + return ; +} + +function HostActorAvatar({ + login, + fetchUrl, + environmentId, + className, +}: { + login: string; + fetchUrl: string; + environmentId: EnvironmentId | null; + className: string | undefined; +}) { + const asset = useAssetUrlState(environmentId, { + _tag: SOURCE_CONTROL_MEDIA_ASSET, + url: fetchUrl, + }); + return ( + + ); +} + +function ActorAvatarImage({ + login, + src, + className, +}: { + login: string; + src: string | null; + className: string | undefined; +}) { + const [failedSrc, setFailedSrc] = useState(null); + return src === null || failedSrc === src ? ( setFailedAvatarUrl(avatarUrl)} + onError={() => setFailedSrc(src)} /> ); } export function PullRequestActorLabel({ actor, + environmentId, className, variant = "label", tooltip = true, profileUrl, }: { actor: PullRequestActor | null; + environmentId?: EnvironmentId | null | undefined; className?: string; variant?: "label" | "avatar"; tooltip?: boolean; @@ -413,7 +469,7 @@ export function PullRequestActorLabel({ const login = actor?.login ?? "ghost"; const label = ( - + {login} diff --git a/packages/contracts/src/assets.ts b/packages/contracts/src/assets.ts index da3eb0e42062f..3391d5b812d9a 100644 --- a/packages/contracts/src/assets.ts +++ b/packages/contracts/src/assets.ts @@ -52,11 +52,12 @@ export const AssetResource = Schema.Union([ Schema.TaggedStruct("native-app-icon", { app: ToolActivityNativeAppReference, }), - // Media a change request body points at on its host. A private repository serves these only - // to a request that carries a credential, which the client has none of, so the server fetches - // them with the credential the host's driver holds for the repository at `cwd`. + // Media a change request points at on its host. A private repository serves these only to a + // request that carries a credential, which the client has none of, so the server fetches them + // with the credential the host's driver holds. The server ignores `cwd`; servers before it + // became optional require it, so clients still send it when they know the repository. Schema.TaggedStruct(SOURCE_CONTROL_MEDIA_ASSET, { - cwd: TrimmedNonEmptyString.check(Schema.isMaxLength(ASSET_PATH_MAX_LENGTH)), + cwd: Schema.optional(TrimmedNonEmptyString.check(Schema.isMaxLength(ASSET_PATH_MAX_LENGTH))), url: TrimmedNonEmptyString.check(Schema.isMaxLength(2048)), }), ]); diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsCli.test.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsCli.test.ts index ff2ddd0339979..e6aca6802fc7b 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsCli.test.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsCli.test.ts @@ -4,6 +4,7 @@ import * as Effect from "effect/Effect"; import * as Layer from "effect/Layer"; import * as Option from "effect/Option"; import * as PlatformError from "effect/PlatformError"; +import * as TestClock from "effect/testing/TestClock"; import { ChildProcessSpawner } from "effect/process"; import { VcsProcessExitError, VcsProcessSpawnError } from "@t3tools/contracts"; @@ -271,4 +272,113 @@ describe("AzureDevOpsCli.layer", () => { assert.strictEqual(error.cause, cause); }).pipe(Effect.provide(layer)), ); + + describe("getIdentityAvatar", () => { + const identity = { + organizationUrl: "https://dev.azure.com/acme", + identityId: "15f0488e-b980-68ca-9d71-8a387c166d09", + }; + const png = Uint8Array.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]); + const graphValue = (value: string) => Effect.succeed(processOutput(JSON.stringify({ value }))); + const answerAvatarLookup = () => { + mockRun + .mockReturnValueOnce(graphValue("aad.descriptor")) + .mockReturnValueOnce(graphValue(Buffer.from(png).toString("base64"))); + }; + + it.effect("resolves the descriptor, decodes the image, and caches it", () => + Effect.gen(function* () { + answerAvatarLookup(); + const az = yield* AzureDevOpsCli.AzureDevOpsCli; + + const first = yield* az.getIdentityAvatar({ cwd: "/home", identity }); + const second = yield* az.getIdentityAvatar({ cwd: "/home", identity: { ...identity } }); + + assert.deepStrictEqual(first, { bytes: png, mimeType: "image/png" }); + assert.strictEqual(second, first); + assert.strictEqual(mockRun.mock.calls.length, 2); + assert.deepStrictEqual( + mockRun.mock.calls.map(([input]) => input.args), + [ + [ + "devops", + "invoke", + "--org", + "https://dev.azure.com/acme", + "--area", + "graph", + "--resource", + "descriptors", + "--route-parameters", + "storageKey=15f0488e-b980-68ca-9d71-8a387c166d09", + "--api-version", + "7.1-preview", + "--only-show-errors", + "--output", + "json", + ], + [ + "devops", + "invoke", + "--org", + "https://dev.azure.com/acme", + "--area", + "graph", + "--resource", + "avatars", + "--route-parameters", + "subjectDescriptor=aad.descriptor", + "--query-parameters", + "size=medium", + "--api-version", + "7.1-preview", + "--only-show-errors", + "--output", + "json", + ], + ], + ); + }).pipe(Effect.provide(layer)), + ); + + it.effect("returns null for bytes that are not an image", () => + Effect.gen(function* () { + mockRun + .mockReturnValueOnce(graphValue("aad.descriptor")) + .mockReturnValueOnce(graphValue(Buffer.from("").toString("base64"))); + const az = yield* AzureDevOpsCli.AzureDevOpsCli; + + assert.isNull(yield* az.getIdentityAvatar({ cwd: "/home", identity })); + }).pipe(Effect.provide(layer)), + ); + + it.effect("asks again soon after an authentication failure", () => + Effect.gen(function* () { + mockRun.mockReturnValueOnce( + Effect.fail( + new VcsProcessExitError({ + operation: "AzureDevOpsCli.execute", + command: "az", + cwd: "/home", + argumentCount: 15, + exitCode: 1, + detail: "Please run 'az login' to setup account.", + failureKind: "authentication", + }), + ), + ); + const az = yield* AzureDevOpsCli.AzureDevOpsCli; + + const error = yield* az.getIdentityAvatar({ cwd: "/home", identity }).pipe(Effect.flip); + assert.instanceOf(error, AzureDevOpsCli.AzureDevOpsCliAuthenticationError); + yield* az.getIdentityAvatar({ cwd: "/home", identity }).pipe(Effect.flip); + assert.strictEqual(mockRun.mock.calls.length, 1); + + yield* TestClock.adjust("1 minute"); + answerAvatarLookup(); + const avatar = yield* az.getIdentityAvatar({ cwd: "/home", identity }); + assert.strictEqual(avatar?.mimeType, "image/png"); + }).pipe(Effect.provide(layer)), + ); + }); }); diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsCli.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsCli.ts index 73006ac3053f5..62b4fbc655152 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsCli.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsCli.ts @@ -1,5 +1,8 @@ +import * as Cache from "effect/Cache"; import * as Context from "effect/Context"; import * as Effect from "effect/Effect"; +import * as Base64 from "effect/encoding/Base64"; +import * as Exit from "effect/Exit"; import * as Layer from "effect/Layer"; import * as PlatformError from "effect/PlatformError"; import * as Result from "effect/Result"; @@ -13,6 +16,7 @@ import { } from "@t3tools/contracts"; import * as SourceControlHost from "@t3tools/source-control-core/server/SourceControlHost"; +import type { AzureDevOpsAvatarIdentity } from "../shared/azureDevOpsAvatar.ts"; import { decodeAzureDevOpsPullRequestJson, decodeAzureDevOpsPullRequestListJson, @@ -204,6 +208,11 @@ export const AzureDevOpsCliError = Schema.Union([ ]); export type AzureDevOpsCliError = typeof AzureDevOpsCliError.Type; +export interface AzureDevOpsIdentityAvatar { + readonly bytes: Uint8Array; + readonly mimeType: "image/png" | "image/jpeg" | "image/gif"; +} + export interface AzureDevOpsRepositoryCloneUrls { readonly nameWithOwner: string; readonly url: string; @@ -263,6 +272,15 @@ export class AzureDevOpsCli extends Context.Service< readonly reference: string; readonly remoteName?: string; }) => Effect.Effect; + + /** + * Cached per identity. `null` means Azure answered but had no image `az` could decode; the + * error channel keeps an unauthenticated or missing `az` distinguishable for logging. + */ + readonly getIdentityAvatar: (input: { + readonly cwd: string; + readonly identity: AzureDevOpsAvatarIdentity; + }) => Effect.Effect; } >()("@t3tools/source-control-azure-devops/server/AzureDevOpsCli") {} @@ -362,6 +380,25 @@ const classifyAzFailure = (stderr: string): VcsProcessExitFailureKind | null => : null; }; +const AVATAR_CACHE_CAPACITY = 500; +// People rarely change their picture, and a lookup costs two `az` processes. +const AVATAR_TTL = "6 hours"; +// Short enough that signing in to `az` shows pictures without a server restart. +const AVATAR_FAILURE_TTL = "1 minute"; +const GRAPH_API_VERSION = "7.1-preview"; + +const GraphValueJson = Schema.fromJsonString(Schema.Struct({ value: Schema.String })); +const decodeGraphValue = Schema.decodeUnknownOption(GraphValueJson); + +function sniffAvatarMimeType(bytes: Uint8Array): AzureDevOpsIdentityAvatar["mimeType"] | null { + if (bytes[0] === 0x89 && bytes[1] === 0x50 && bytes[2] === 0x4e && bytes[3] === 0x47) { + return "image/png"; + } + if (bytes[0] === 0xff && bytes[1] === 0xd8 && bytes[2] === 0xff) return "image/jpeg"; + if (bytes[0] === 0x47 && bytes[1] === 0x49 && bytes[2] === 0x46) return "image/gif"; + return null; +} + /** @public */ export const make = Effect.gen(function* () { const { process } = yield* SourceControlHost.SourceControlHost; @@ -398,6 +435,79 @@ export const make = Effect.gen(function* () { args: [...input.args, "--only-show-errors", "--output", "json"], }); + // `az devops invoke` resolves the credential the same way every `az devops` command does: + // `AZURE_DEVOPS_EXT_PAT`, then a PAT saved by `az devops login`, then the `az login` session. + // The identity image URL itself only accepts a browser session, and the Graph avatar endpoint + // wants a subject descriptor, which pull request reviewers do not carry. + const invokeGraph = (input: { + readonly cwd: string; + readonly organizationUrl: string; + readonly resource: "descriptors" | "avatars"; + readonly routeParameter: string; + readonly queryParameters?: ReadonlyArray; + }) => + executeJson({ + cwd: input.cwd, + args: [ + "devops", + "invoke", + "--org", + input.organizationUrl, + "--area", + "graph", + "--resource", + input.resource, + "--route-parameters", + input.routeParameter, + ...(input.queryParameters === undefined + ? [] + : ["--query-parameters", ...input.queryParameters]), + "--api-version", + GRAPH_API_VERSION, + ], + }).pipe( + Effect.map((result) => { + const decoded = decodeGraphValue(result.stdout); + return decoded._tag === "Some" ? decoded.value.value : null; + }), + ); + + const avatarCache = yield* Cache.makeWith( + ({ cwd, identity }: Parameters[0]) => + invokeGraph({ + cwd, + organizationUrl: identity.organizationUrl, + resource: "descriptors", + routeParameter: `storageKey=${identity.identityId}`, + }).pipe( + Effect.flatMap((descriptor) => + descriptor === null + ? Effect.succeed(null) + : invokeGraph({ + cwd, + organizationUrl: identity.organizationUrl, + resource: "avatars", + routeParameter: `subjectDescriptor=${descriptor}`, + // 44 px. `large` is 220 px and about twenty times the bytes, for avatars drawn at + // 14 to 32 px. + queryParameters: ["size=medium"], + }), + ), + Effect.map((encoded): AzureDevOpsIdentityAvatar | null => { + if (encoded === null) return null; + const bytes = Base64.decode(encoded); + if (!Result.isSuccess(bytes)) return null; + const mimeType = sniffAvatarMimeType(bytes.success); + return mimeType === null ? null : { bytes: bytes.success, mimeType }; + }), + ), + { + capacity: AVATAR_CACHE_CAPACITY, + timeToLive: (exit) => + Exit.isSuccess(exit) && exit.value !== null ? AVATAR_TTL : AVATAR_FAILURE_TTL, + }, + ); + return AzureDevOpsCli.of({ execute, listPullRequests: (input) => @@ -559,6 +669,14 @@ export const make = Effect.gen(function* () { input.remoteName ?? "origin", ], }).pipe(Effect.asVoid), + getIdentityAvatar: (input) => + Cache.get(avatarCache, { + cwd: input.cwd, + identity: { + organizationUrl: input.identity.organizationUrl, + identityId: input.identity.identityId, + }, + }), }); }); diff --git a/packages/source-control-azure-devops/src/server/azureDevOpsAvatarFetch.test.ts b/packages/source-control-azure-devops/src/server/azureDevOpsAvatarFetch.test.ts new file mode 100644 index 0000000000000..fafb0edbd8fb8 --- /dev/null +++ b/packages/source-control-azure-devops/src/server/azureDevOpsAvatarFetch.test.ts @@ -0,0 +1,54 @@ +import { describe, expect, it } from "@effect/vitest"; +import * as Effect from "effect/Effect"; +import * as Layer from "effect/Layer"; + +import { definition } from "../shared/definition.ts"; + +import * as AzureDevOpsAvatarFetch from "./azureDevOpsAvatarFetch.ts"; +import * as AzureDevOpsCli from "./AzureDevOpsCli.ts"; + +const ID = "15f0488e-b980-68ca-9d71-8a387c166d09"; +const PNG = new Uint8Array([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]); + +describe("AzureDevOpsAvatarFetch", () => { + it.effect("serves the avatar `az` reads for the URL the definition claimed", () => { + const lookups: Array = []; + return Effect.gen(function* () { + const fetchMedia = yield* AzureDevOpsAvatarFetch.make; + const media = definition.mediaUrl?.( + new URL(`https://acme.visualstudio.com/_api/_common/identityImage?id=${ID}`), + ); + expect(media?.fetchUrl).toBeDefined(); + if (media == null) return; + + const response = yield* fetchMedia({ url: media.fetchUrl, headers: {} }); + expect(response?.status).toBe(200); + if (response == null) return; + expect(response.headers["content-type"]).toBe("image/png"); + expect(new Uint8Array(yield* response.arrayBuffer)).toEqual(PNG); + expect(lookups).toEqual([ + { organizationUrl: "https://acme.visualstudio.com", identityId: ID }, + ]); + + // Azure answered without an image `az` could decode. + const missing = yield* fetchMedia({ + url: `https://dev.azure.com/acme/_api/_common/identityImage?id=${ID}`, + headers: {}, + }); + expect(missing?.status).toBe(404); + }).pipe( + Effect.provide( + Layer.mock(AzureDevOpsCli.AzureDevOpsCli)({ + getIdentityAvatar: ({ identity }) => { + lookups.push(identity); + return Effect.succeed( + identity.organizationUrl === "https://acme.visualstudio.com" + ? { bytes: PNG, mimeType: "image/png" as const } + : null, + ); + }, + }), + ), + ); + }); +}); diff --git a/packages/source-control-azure-devops/src/server/azureDevOpsAvatarFetch.ts b/packages/source-control-azure-devops/src/server/azureDevOpsAvatarFetch.ts new file mode 100644 index 0000000000000..a973e6341a7d8 --- /dev/null +++ b/packages/source-control-azure-devops/src/server/azureDevOpsAvatarFetch.ts @@ -0,0 +1,40 @@ +import * as HostProcess from "@t3tools/shared/HostProcess"; +import type { SourceControlMediaRequest } from "@t3tools/source-control-core/server/driver"; +import * as Effect from "effect/Effect"; +import { HttpClientRequest, HttpClientResponse } from "effect/http"; + +import { azureDevOpsAvatarIdentity } from "../shared/azureDevOpsAvatar.ts"; + +import * as AzureDevOpsCli from "./AzureDevOpsCli.ts"; + +export const make = Effect.gen(function* () { + const az = yield* AzureDevOpsCli.AzureDevOpsCli; + + return Effect.fn("AzureDevOpsAvatarFetch.fetchMedia")(function* ( + request: SourceControlMediaRequest, + ) { + const identity = azureDevOpsAvatarIdentity(request.url); + if (identity === null) return null; + const avatar = yield* az + .getIdentityAvatar({ + // `--org` names the organization, so the working directory only has to exist. + cwd: yield* HostProcess.HomeDirectory, + identity, + }) + .pipe( + Effect.tapError((cause) => + Effect.logWarning("Failed to fetch an Azure DevOps avatar.", { + organizationUrl: identity.organizationUrl, + cause, + }), + ), + Effect.orElseSucceed(() => null), + ); + // `az` hands back the whole image, so there is no upstream response to stream through. + const response = + avatar === null + ? new Response(null, { status: 404 }) + : new Response(new Blob([new Uint8Array(avatar.bytes)], { type: avatar.mimeType })); + return HttpClientResponse.fromWeb(HttpClientRequest.get(request.url), response); + }); +}); diff --git a/packages/source-control-azure-devops/src/server/driver.ts b/packages/source-control-azure-devops/src/server/driver.ts index ac1f1a53855a3..08993c88832dc 100644 --- a/packages/source-control-azure-devops/src/server/driver.ts +++ b/packages/source-control-azure-devops/src/server/driver.ts @@ -7,6 +7,7 @@ import * as Layer from "effect/Layer"; import { definition } from "../shared/definition.ts"; +import * as AzureDevOpsAvatarFetch from "./azureDevOpsAvatarFetch.ts"; import * as AzureDevOpsCli from "./AzureDevOpsCli.ts"; import * as AzureDevOpsPullRequestCli from "./AzureDevOpsPullRequestCli.ts"; import * as AzureDevOpsPullRequestProvider from "./AzureDevOpsPullRequestProvider.ts"; @@ -18,6 +19,7 @@ const makeInstance = Effect.all({ sourceControl: AzureDevOpsSourceControlProvider.make, discovery: Effect.succeed(AzureDevOpsSourceControlProvider.discovery), pullRequests: AzureDevOpsPullRequestProvider.make, + fetchMedia: AzureDevOpsAvatarFetch.make, }); export const driver = defineSourceControlDriver({ diff --git a/packages/source-control-azure-devops/src/shared/azureDevOpsAvatar.test.ts b/packages/source-control-azure-devops/src/shared/azureDevOpsAvatar.test.ts new file mode 100644 index 0000000000000..156c6d40392cd --- /dev/null +++ b/packages/source-control-azure-devops/src/shared/azureDevOpsAvatar.test.ts @@ -0,0 +1,50 @@ +import { describe, expect, it } from "vite-plus/test"; + +import { azureDevOpsAvatarIdentity, azureDevOpsAvatarMediaUrl } from "./azureDevOpsAvatar.ts"; + +const ID = "15f0488e-b980-68ca-9d71-8a387c166d09"; + +describe("azureDevOpsAvatarIdentity", () => { + it("reads the organization and identity from both host shapes", () => { + expect( + azureDevOpsAvatarIdentity(`https://dev.azure.com/acme/_api/_common/identityImage?id=${ID}`), + ).toEqual({ organizationUrl: "https://dev.azure.com/acme", identityId: ID }); + expect( + azureDevOpsAvatarIdentity( + `https://acme.visualstudio.com/_api/_common/identityImage?id=${ID.toUpperCase()}`, + ), + ).toEqual({ organizationUrl: "https://acme.visualstudio.com", identityId: ID }); + }); + + it("rejects anything else", () => { + for (const url of [ + `http://dev.azure.com/acme/_api/_common/identityImage?id=${ID}`, + `https://dev.azure.com/_api/_common/identityImage?id=${ID}`, + `https://dev.azure.com/acme/project/_api/_common/identityImage?id=${ID}`, + `https://dev.azure.com/acme/_api/_common/identityImage?id=not-a-guid`, + `https://evil.example/acme/_api/_common/identityImage?id=${ID}`, + `https://evil.visualstudio.com.example/_api/_common/identityImage?id=${ID}`, + "https://avatars.githubusercontent.com/u/1", + "not a url", + ]) { + expect(azureDevOpsAvatarIdentity(url), url).toBeNull(); + } + }); + + it("claims one canonical fetch URL for every spelling of an identity image", () => { + expect( + azureDevOpsAvatarMediaUrl( + new URL( + `https://dev.azure.com/acme/_API/_common/IdentityImage?id=${ID.toUpperCase()}&size=2`, + ), + ), + ).toEqual({ + fetchUrl: `https://dev.azure.com/acme/_api/_common/identityImage?id=${ID}`, + fileName: "avatar", + kind: "image", + }); + expect(azureDevOpsAvatarMediaUrl(new URL("https://avatars.githubusercontent.com/u/1"))).toBe( + null, + ); + }); +}); diff --git a/packages/source-control-azure-devops/src/shared/azureDevOpsAvatar.ts b/packages/source-control-azure-devops/src/shared/azureDevOpsAvatar.ts new file mode 100644 index 0000000000000..b136d8667c924 --- /dev/null +++ b/packages/source-control-azure-devops/src/shared/azureDevOpsAvatar.ts @@ -0,0 +1,50 @@ +import type { SourceControlMediaUrl } from "@t3tools/source-control-core/shared/definition"; + +export interface AzureDevOpsAvatarIdentity { + /** The organization `az devops --org` takes, which also picks the credential `az` signs in with. */ + readonly organizationUrl: string; + readonly identityId: string; +} + +const IDENTITY_IMAGE_PATH = "/_api/_common/identityimage"; +const ORGANIZATION_SEGMENT_PATTERN = /^[a-z0-9][a-z0-9-]*$/iu; +const VISUAL_STUDIO_HOST_PATTERN = /^([a-z0-9][a-z0-9-]*)\.visualstudio\.com$/iu; +const IDENTITY_ID_PATTERN = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/iu; + +function identityFromUrl(url: URL): AzureDevOpsAvatarIdentity | null { + if (url.protocol !== "https:") return null; + const identityId = url.searchParams.get("id")?.toLowerCase() ?? ""; + if (!IDENTITY_ID_PATTERN.test(identityId)) return null; + const host = url.hostname.toLowerCase(); + const path = url.pathname.toLowerCase(); + if (host === "dev.azure.com") { + const separator = path.indexOf("/", 1); + const organization = separator === -1 ? "" : url.pathname.slice(1, separator); + if (!ORGANIZATION_SEGMENT_PATTERN.test(organization)) return null; + if (path.slice(separator) !== IDENTITY_IMAGE_PATH) return null; + return { organizationUrl: `https://dev.azure.com/${organization}`, identityId }; + } + const legacyHost = VISUAL_STUDIO_HOST_PATTERN.exec(host); + if (legacyHost === null || path !== IDENTITY_IMAGE_PATH) return null; + return { organizationUrl: `https://${legacyHost[1]}.visualstudio.com`, identityId }; +} + +/** + * Reads the identity image URL Azure DevOps puts on pull request authors and reviewers. Only the + * shapes `az` can authenticate against are accepted, so nothing else reaches the server's fetch. + */ +export function azureDevOpsAvatarIdentity(source: string): AzureDevOpsAvatarIdentity | null { + return URL.canParse(source) ? identityFromUrl(new URL(source)) : null; +} + +/** Identity images load only for a signed-in browser session, so the server fetches them. */ +export function azureDevOpsAvatarMediaUrl(url: URL): SourceControlMediaUrl | null { + const identity = identityFromUrl(url); + return identity === null + ? null + : { + fetchUrl: `${identity.organizationUrl}/_api/_common/identityImage?id=${identity.identityId}`, + fileName: "avatar", + kind: "image", + }; +} diff --git a/packages/source-control-azure-devops/src/shared/definition.ts b/packages/source-control-azure-devops/src/shared/definition.ts index b43adccedc846..883250d4d4ac2 100644 --- a/packages/source-control-azure-devops/src/shared/definition.ts +++ b/packages/source-control-azure-devops/src/shared/definition.ts @@ -6,6 +6,7 @@ import { parseChangeRequestPath, remoteWebOrigin, } from "@t3tools/source-control-core/shared/definition"; +import { azureDevOpsAvatarMediaUrl } from "./azureDevOpsAvatar.ts"; import { icon } from "./icon.ts"; // Azure DevOps Server runs on any hostname; its `_git` route identifies it. @@ -145,4 +146,5 @@ export const definition = defineSourceControl({ ); }, canReadChangeRequestOnHost: () => false, + mediaUrl: azureDevOpsAvatarMediaUrl, }); From ccd5331064bd1c21069c292cbf280b83649959bb Mon Sep 17 00:00:00 2001 From: Pavel Kroupa <63880977+Tabonx@users.noreply.github.com> Date: Mon, 26 Oct 2026 08:14:30 +0100 Subject: [PATCH 2/6] fix(server): browsers retry host media the host refused A failed source control media response kept the signed URL's max-age, so a browser held on to an Azure DevOps avatar 404 for up to an hour, even after the user signed in to `az`. Failures are now private and no-store. --- apps/server/src/assets/AssetAccess.test.ts | 30 +++++++++++++++++++ .../src/assets/sourceControlMediaResponse.ts | 19 ++++++------ 2 files changed, 40 insertions(+), 9 deletions(-) diff --git a/apps/server/src/assets/AssetAccess.test.ts b/apps/server/src/assets/AssetAccess.test.ts index 5fa4f98ccd99c..e9db5054cb7d6 100644 --- a/apps/server/src/assets/AssetAccess.test.ts +++ b/apps/server/src/assets/AssetAccess.test.ts @@ -1447,6 +1447,36 @@ describe("AssetAccess", () => { }).pipe(Effect.provide(Layer.merge(layerTest, layerDrivers)), Effect.scoped); }); + it.effect("lets the browser retry host media the host refused", () => { + const url = + "https://dev.azure.com/acme/_api/_common/identityImage?id=15f0488e-b980-68ca-9d71-8a387c166d09"; + const media = sourceControlDefinitions.mediaUrl(url)!; + const layerDrivers = Layer.succeed(BuiltInDrivers.SourceControlDrivers, [ + { + driver: { kind: media.definition.kind, definition: media.definition }, + instance: fakeSourceControlDriverInstance(media.definition.kind, { + pullRequests: null, + fetchMedia: (request) => + Effect.succeed( + HttpClientResponse.fromWeb( + HttpClientRequest.get(request.url), + new Response(null, { status: 404 }), + ), + ), + }), + }, + ]); + return Effect.gen(function* () { + // A host that refused before sign-in serves the media right after it. + const response = yield* sourceControlMediaResponse( + { url: media.fetchUrl, expiresAt: Date.UTC(2100, 0, 1) }, + {}, + ); + expect(response.status).toBe(404); + expect(response.headers["cache-control"]).toBe("private, no-store"); + }).pipe(Effect.provide(Layer.merge(layerTest, layerDrivers)), Effect.scoped); + }); + it.effect("signs host media a client asks for without a repository", () => Effect.gen(function* () { // A pull request list draws avatars with no repository at hand. diff --git a/apps/server/src/assets/sourceControlMediaResponse.ts b/apps/server/src/assets/sourceControlMediaResponse.ts index f6abe00edb5fc..91b3ff98a226d 100644 --- a/apps/server/src/assets/sourceControlMediaResponse.ts +++ b/apps/server/src/assets/sourceControlMediaResponse.ts @@ -34,12 +34,18 @@ export const sourceControlMediaResponse = Effect.fn( remainingSeconds > 0 ? `private, max-age=${remainingSeconds}` : "private, no-store", "x-content-type-options": "nosniff", }; + // A host can refuse until the user signs in, so a failure must not outlive this request. + const failure = (status: number) => + HttpServerResponse.empty({ + status, + headers: { ...headers, "cache-control": "private, no-store" }, + }); const media = sourceControlDefinitions.mediaUrl(asset.url); const drivers = yield* BuiltInDrivers.SourceControlDrivers; const fetchMedia = drivers.find(({ driver }) => driver.kind === media?.definition.kind)?.instance .fetchMedia; if (media === null || fetchMedia === undefined) { - return HttpServerResponse.empty({ status: 404, headers }); + return failure(404); } const forwarded: Record = {}; for (const name of FORWARDED_REQUEST_HEADERS) { @@ -47,22 +53,17 @@ export const sourceControlMediaResponse = Effect.fn( if (value !== undefined) forwarded[name] = value; } const response = yield* fetchMedia({ url: asset.url, headers: forwarded }); - if (response === null) return HttpServerResponse.empty({ status: 502, headers }); + if (response === null) return failure(502); // It carries none of the upstream entity headers: a `content-length` with no body behind it // holds the connection open until the browser gives up on it. - if (response.status >= 400) { - return HttpServerResponse.empty({ - status: response.status >= 500 ? 502 : response.status, - headers, - }); - } + if (response.status >= 400) return failure(response.status >= 500 ? 502 : response.status); const upstreamType = response.headers["content-type"]?.split(";", 1)[0]?.trim().toLowerCase() ?? ""; const contentType = MEDIA_CONTENT_TYPE_PATTERN.test(upstreamType) ? upstreamType : Option.getOrElse(Mime.getType(media.fileName), () => "").toLowerCase(); if (!MEDIA_CONTENT_TYPE_PATTERN.test(contentType)) { - return HttpServerResponse.empty({ status: 415, headers }); + return failure(415); } for (const name of FORWARDED_RESPONSE_HEADERS) { const value = response.headers[name]; From c67e7ca9dadb15020cee15738c573306858abc72 Mon Sep 17 00:00:00 2001 From: maria-rcks Date: Mon, 26 Oct 2026 17:48:23 +0000 Subject: [PATCH 3/6] fix(azure-devops): trace avatar lookups --- .../src/server/AzureDevOpsCli.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsCli.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsCli.ts index 28cef0afd2e23..ea5adcec15e72 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsCli.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsCli.ts @@ -675,14 +675,15 @@ export const make = Effect.gen(function* () { ], }).pipe(Effect.asVoid); }), - getIdentityAvatar: (input) => - Cache.get(avatarCache, { + getIdentityAvatar: Effect.fn("AzureDevOpsCli.getIdentityAvatar")(function (input) { + return Cache.get(avatarCache, { cwd: input.cwd, identity: { organizationUrl: input.identity.organizationUrl, identityId: input.identity.identityId, }, - }), + }); + }), }); }); From 5d32d9c75333581c077391cee1ad51adb54f2079 Mon Sep 17 00:00:00 2001 From: Pavel Kroupa <63880977+Tabonx@users.noreply.github.com> Date: Sun, 1 Nov 2026 20:38:20 +0100 Subject: [PATCH 4/6] fix(azure-devops): use the released Graph API version --- .../src/server/AzureDevOpsCli.test.ts | 4 ++-- .../source-control-azure-devops/src/server/AzureDevOpsCli.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsCli.test.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsCli.test.ts index 709f871180f33..6836fcd9db6af 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsCli.test.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsCli.test.ts @@ -347,7 +347,7 @@ describe("AzureDevOpsCli.layer", () => { "--route-parameters", "storageKey=15f0488e-b980-68ca-9d71-8a387c166d09", "--api-version", - "7.1-preview", + "7.1", "--only-show-errors", "--output", "json", @@ -366,7 +366,7 @@ describe("AzureDevOpsCli.layer", () => { "--query-parameters", "size=medium", "--api-version", - "7.1-preview", + "7.1", "--only-show-errors", "--output", "json", diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsCli.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsCli.ts index 107942adea6e7..3b3de356f8eda 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsCli.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsCli.ts @@ -385,7 +385,7 @@ const AVATAR_CACHE_CAPACITY = 500; const AVATAR_TTL = "6 hours"; // Short enough that signing in to `az` shows pictures without a server restart. const AVATAR_FAILURE_TTL = "1 minute"; -const GRAPH_API_VERSION = "7.1-preview"; +const GRAPH_API_VERSION = "7.1"; const GraphValueJson = Schema.fromJsonString(Schema.Struct({ value: Schema.String })); const decodeGraphValue = Schema.decodeUnknownOption(GraphValueJson); From 8c2f9a3763c8388933e7d2cfe95e2e441531a2d3 Mon Sep 17 00:00:00 2001 From: maria-rcks Date: Thu, 5 Nov 2026 23:01:57 +0000 Subject: [PATCH 5/6] test(server): source-control media tests cover the cwd fallback after merging main --- apps/server/src/assets/AssetAccess.test.ts | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/apps/server/src/assets/AssetAccess.test.ts b/apps/server/src/assets/AssetAccess.test.ts index f13db9e44dcdc..4c9227da13d18 100644 --- a/apps/server/src/assets/AssetAccess.test.ts +++ b/apps/server/src/assets/AssetAccess.test.ts @@ -1548,6 +1548,7 @@ describe("AssetAccess", () => { expect(asset).toEqual({ kind: "source-control-media", url: media.fetchUrl, + cwd: "/repo", expiresAt: issued.expiresAt, }); if (asset?.kind !== "source-control-media") return; @@ -1600,7 +1601,16 @@ describe("AssetAccess", () => { ); expect(response.status).toBe(404); expect(response.headers["cache-control"]).toBe("private, no-store"); - }).pipe(Effect.provide(Layer.merge(layerTest, layerDrivers)), Effect.scoped); + }).pipe( + Effect.provide( + Layer.mergeAll( + layerTest, + layerDrivers, + Layer.mock(RepositoryIdentityResolver.RepositoryIdentityResolver)({}), + ), + ), + Effect.scoped, + ); }); it.effect("signs host media a client asks for without a repository", () => From d3761d7d5bc6f038ead0243032f5c68637771b60 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Thu, 12 Nov 2026 22:18:25 +0000 Subject: [PATCH 6/6] fix(azure-devops): cap concurrent avatar lookups Each uncached avatar runs two az processes in turn, and a list with many authors started them all at once. A semaphore now admits two lookups at a time; cache hits skip it. AI-assisted; verified with the tests and tools listed above. --- .../src/server/AzureDevOpsCli.test.ts | 38 +++++++++++++++++++ .../src/server/AzureDevOpsCli.ts | 7 ++++ 2 files changed, 45 insertions(+) diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsCli.test.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsCli.test.ts index e584c7342a2bb..5dab7c76c71a1 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsCli.test.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsCli.test.ts @@ -408,6 +408,44 @@ describe("AzureDevOpsCli.layer", () => { }).pipe(Effect.provide(layer)), ); + it.effect("caps how many `az` processes concurrent avatar lookups start", () => + Effect.gen(function* () { + let inFlight = 0; + let peakInFlight = 0; + mockRun.mockImplementation((input) => + Effect.gen(function* () { + inFlight += 1; + peakInFlight = Math.max(peakInFlight, inFlight); + // Yield like a subprocess so the other lookups get a chance to start. + yield* Effect.yieldNow; + yield* Effect.yieldNow; + inFlight -= 1; + return processOutput( + JSON.stringify({ + value: input.args.includes("descriptors") + ? "aad.descriptor" + : Buffer.from(png).toString("base64"), + }), + ); + }), + ); + const az = yield* AzureDevOpsCli.AzureDevOpsCli; + + yield* Effect.all( + Array.from({ length: 8 }, (_, index) => + az.getIdentityAvatar({ + cwd: "/home", + identity: { ...identity, identityId: identity.identityId.replace(/.$/u, `${index}`) }, + }), + ), + { concurrency: "unbounded" }, + ); + + assert.strictEqual(mockRun.mock.calls.length, 16); + assert.isAtMost(peakInFlight, AzureDevOpsCli.MAX_AVATAR_LOOKUPS); + }).pipe(Effect.provide(layer)), + ); + it.effect("returns null for bytes that are not an image", () => Effect.gen(function* () { mockRun diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsCli.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsCli.ts index 0cfa318f84f2e..7ae92c62f3433 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsCli.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsCli.ts @@ -7,6 +7,7 @@ import * as Layer from "effect/Layer"; import * as PlatformError from "effect/PlatformError"; import * as Result from "effect/Result"; import * as Schema from "effect/Schema"; +import * as Semaphore from "effect/Semaphore"; import { NonNegativeInt, TrimmedNonEmptyString, @@ -382,6 +383,10 @@ const classifyAzFailure = (stderr: string): VcsProcessExitFailureKind | null => }; const AVATAR_CACHE_CAPACITY = 500; +// Each lookup runs two `az` Python processes in turn; a list with many authors must not start +// them all at once on the host that also runs the user's agents. +/** @public */ +export const MAX_AVATAR_LOOKUPS = 2; // People rarely change their picture, and a lookup costs two `az` processes. const AVATAR_TTL = "6 hours"; // Short enough that signing in to `az` shows pictures without a server restart. @@ -473,6 +478,7 @@ export const make = Effect.gen(function* () { }), ); + const avatarLookups = yield* Semaphore.make(MAX_AVATAR_LOOKUPS); const avatarCache = yield* Cache.makeWith( ({ cwd, identity }: Parameters[0]) => invokeGraph({ @@ -501,6 +507,7 @@ export const make = Effect.gen(function* () { const mimeType = sniffAvatarMimeType(bytes.success); return mimeType === null ? null : { bytes: bytes.success, mimeType }; }), + avatarLookups.withPermits(1), ), { capacity: AVATAR_CACHE_CAPACITY,