diff --git a/apps/mobile/src/features/pull-requests/PullRequestDiscussionScreen.tsx b/apps/mobile/src/features/pull-requests/PullRequestDiscussionScreen.tsx index 9e4231ad65dd2..40a7d0b675af4 100644 --- a/apps/mobile/src/features/pull-requests/PullRequestDiscussionScreen.tsx +++ b/apps/mobile/src/features/pull-requests/PullRequestDiscussionScreen.tsx @@ -1,3 +1,8 @@ +import { + parsePullRequestAnchor, + pullRequestAnchorMatchesUrl, + resolvePullRequestReviewAnchor, +} from "@t3tools/shared/pullRequestAnchor"; import { AppActivityIndicator } from "../../components/ui/AppActivityIndicator"; import { useAtomRefresh } from "@effect/atom-react"; import { useFocusEffect, useNavigation, type StaticScreenProps } from "@react-navigation/native"; @@ -8,7 +13,7 @@ import { import { AsyncResult } from "effect/reactivity"; import type { PullRequestThreadComment } from "@t3tools/contracts"; import { useRef, useState } from "react"; -import { ScrollView, View } from "react-native"; +import { ScrollView, View, type ScrollViewInstance } from "react-native"; import { KeyboardAvoidingView, useKeyboardState } from "react-native-keyboard-controller"; import { NativeScrollViewMarker as ScrollViewMarker } from "../../native/NativeScrollViewMarker"; import { useSafeAreaInsets } from "react-native-safe-area-context"; @@ -138,11 +143,27 @@ function Discussion({ params }: { params: PullRequestParams & { readonly discuss [...extra, ...(thread?.comments ?? [])].map((comment) => [comment.id, comment]), ).values(), ].sort((left, right) => left.createdAt.localeCompare(right.createdAt)); + const scrollRef = useRef(null); + const pendingTop = useRef(null); + const positionedComment = useRef(null); + const location = parsePullRequestAnchor(params.navigationUrl); + const anchor = + location?.kind === "timeline" + ? resolvePullRequestReviewAnchor(location.anchor, [], [{ comments }]) + : null; const canReply = permissions.canReply && Boolean(thread); const resolveLabel = thread?.isResolved ? "Unresolve" : "Resolve"; return ( - + { + pendingTop.current = null; + }} + > { + if (pendingTop.current === null) return; + scrollRef.current?.scrollTo({ y: pendingTop.current, animated: false }); + pendingTop.current = null; + }} className="flex-1" contentInsetAdjustmentBehavior="automatic" keyboardShouldPersistTaps="handled" @@ -192,7 +219,25 @@ function Discussion({ params }: { params: PullRequestParams & { readonly discuss This discussion is no longer available. ) : null} {comments.map((comment) => ( - + { + if ( + anchor === null || + !pullRequestAnchorMatchesUrl(anchor, comment.url) || + positionedComment.current === comment.id + ) + return; + positionedComment.current = comment.id; + pendingTop.current = event.nativeEvent.layout.y; + scrollRef.current?.scrollTo({ y: pendingTop.current, animated: false }); + }} + className={ + anchor !== null && pullRequestAnchorMatchesUrl(anchor, comment.url) + ? "-mx-3 gap-3 rounded-2xl border border-primary bg-primary/10 px-3 py-4" + : "gap-3 border-b border-border-subtle py-4" + } + > diff --git a/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx b/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx index f78cd9caa58cd..a58de3b87a781 100644 --- a/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx +++ b/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx @@ -1,3 +1,10 @@ +import { + parsePullRequestAnchor, + pullRequestAnchorMatchesUrl, + pullRequestDiffPathMatches, + pullRequestLinkCommit, + resolvePullRequestReviewAnchor, +} from "@t3tools/shared/pullRequestAnchor"; import { AppActivityIndicator } from "../../components/ui/AppActivityIndicator"; import { Chip } from "../../components/ui/Chip"; import { pullRequestTabOffset } from "./pullRequestTabScroll"; @@ -154,8 +161,34 @@ function usePullRequestActivityRows(params: PullRequestParams, filter: ActivityF }); return rows; })(); + const location = parsePullRequestAnchor(params.navigationUrl); + const anchor = + location?.kind !== "timeline" + ? null + : result.data + ? resolvePullRequestReviewAnchor( + location.anchor, + result.data.comments, + result.data.reviewThreads, + ) + : location.anchor; + const targetComment = anchor + ? result.data?.comments.find((comment) => pullRequestAnchorMatchesUrl(anchor, comment.url)) + : null; + const targetThread = anchor + ? result.data?.reviewThreads.find((thread) => + thread.comments.some((comment) => pullRequestAnchorMatchesUrl(anchor, comment.url)), + ) + : null; + const targetKey = targetThread + ? `discussion:${targetThread.id}` + : targetComment + ? `comment:${targetComment.id}` + : null; return { rows, + targetKey, + targetThread, hiddenCount: filtered?.hiddenCount ?? 0, pending: result.isPending && !result.data, error: result.error, @@ -334,7 +367,10 @@ export function PullRequestRouteScreen({ route }: StaticScreenProps("overview"); + const [chosenSection, setSection] = useState<"overview" | "activity" | "guide">(() => { + const kind = parsePullRequestAnchor(params.navigationUrl)?.kind; + return kind === "timeline" || kind === "commits" ? "activity" : "overview"; + }); const requestedSection = useRef(chosenSection); const [listAnimationsReady, setListAnimationsReady] = useState(false); const [descriptionExpanded, setDescriptionExpanded] = useState(false); @@ -345,7 +381,14 @@ export function PullRequestRouteScreen({ route }: StaticScreenProps !guidesEnabled && tab === "guide" ? "overview" : tab; const section = shown(chosenSection); - const [activityFilter, setActivityFilter] = useState(DEFAULT_ACTIVITY_FILTER); + const [activityFilter, setActivityFilter] = useState(() => { + const kind = parsePullRequestAnchor(params.navigationUrl)?.kind; + return kind === "timeline" + ? { ...DEFAULT_ACTIVITY_FILTER, bots: true, resolved: true } + : kind === "commits" + ? { ...DEFAULT_ACTIVITY_FILTER, kind: "commits" } + : DEFAULT_ACTIVITY_FILTER; + }); const activityContent = usePullRequestActivityRows(params, activityFilter); const scrollOffsets = useRef>({}); // The offset of the tab last shown, and which tab that was: a tab standing in for a hidden guide @@ -386,6 +429,74 @@ export function PullRequestRouteScreen({ route }: StaticScreenProps { + const target = parsePullRequestAnchor(params.navigationUrl); + if (target?.kind === "diff" || target?.kind === "code") { + const commit = pullRequestLinkCommit(params.navigationUrl); + navigation.navigate("PullRequestFile", { + ...params, + path: "", + ...(commit ? { commit } : {}), + }); + } else if (target?.kind === "timeline") { + selectSection("activity"); + setActivityFilter((filter) => ({ ...filter, kind: "all", bots: true, resolved: true })); + } else if (target?.kind === "commits") { + selectSection("activity"); + setActivityFilter((filter) => ({ ...filter, kind: "commits" })); + } else if (target?.kind === "checks") { + selectSection("overview"); + // Checks head Overview, which a reused screen may have scrolled past. + scrollViews.current.overview?.scrollToOffset({ offset: -contentTopInset, animated: false }); + } else if (target?.kind === "summary") { + selectSection("overview"); + } + }); + // oxlint-disable-next-line t3code/no-unreasoned-state-sync -- a link selects its native PR section or diff even when the route is reused + useEffect(() => { + // oxlint-disable-next-line react/set-state-in-effect -- URL navigation selects the PR section and makes its target visible + followLink(); + // oxlint-disable-next-line react/exhaustive-effect-dependencies -- each clicked URL is a fresh navigation request + }, [params.navigationUrl, params.navigationId]); + + const activityTargetIndex = activityContent.rows.findIndex( + (row) => row.key === activityContent.targetKey, + ); + const openedDiscussion = useRef(null); + const positionedActivity = useRef(null); + // Rows above the target measure in after the first jump, so it is held until the user scrolls. + const heldActivityRow = useRef(null); + const positionActivityRow = (index: number) => + void scrollViews.current.activity?.scrollToIndex({ + index, + viewPosition: 0, + viewOffset: contentTopInset + 12, + animated: false, + }); + const revealActivity = useEffectEvent(() => { + const key = `${params.navigationUrl}:${params.navigationId ?? 0}`; + if (activityContent.targetThread && openedDiscussion.current !== key) { + openedDiscussion.current = key; + navigation.navigate("PullRequestDiscussion", { + ...params, + discussionId: activityContent.targetThread.id, + }); + return; + } + // Later filter or refresh changes move the row; they must not pull the user back to it. + if (activityTargetIndex >= 0 && positionedActivity.current !== key) { + positionedActivity.current = key; + // Its top sits under the app bar; centering a long comment pushed its start off screen. + heldActivityRow.current = activityTargetIndex + 1; + positionActivityRow(activityTargetIndex + 1); + } + }); + const targetThreadId = activityContent.targetThread?.id; + // oxlint-disable-next-line t3code/no-unreasoned-state-sync -- reveals the requested activity once its asynchronous rows arrive + useEffect(() => { + revealActivity(); + // oxlint-disable-next-line react/exhaustive-effect-dependencies -- reruns when activity resolves the URL target, even a thread the filter hides + }, [params.navigationUrl, params.navigationId, activityTargetIndex, targetThreadId]); const tabContent = activityContent; const filesY = useRef(0); const descriptionY = useRef(0); @@ -596,6 +707,13 @@ export function PullRequestRouteScreen({ route }: StaticScreenProps { + heldActivityRow.current = null; + } + : undefined + } onScroll={(event) => { const offset = event.nativeEvent.contentOffset.y; scrollOffsets.current[tab] = offset; @@ -619,6 +737,13 @@ export function PullRequestRouteScreen({ route }: StaticScreenProps { + const held = heldActivityRow.current; + if (tab === "activity" && held !== null) { + // A filter or refresh that moved the row ends the hold. + if (held !== activityTargetIndex + 1) heldActivityRow.current = null; + else positionActivityRow(held); + return; + } if ( tab !== "overview" || section !== "overview" || @@ -848,9 +973,20 @@ function PullRequestFileRouteContent({ route }: PullRequestFileRouteProps) { (detail.data ?? route.params.initialSummary)?.provider, ).changeRequest; const result = usePullRequestDiff(route.params, terminology); + // Other screens open files with the link's params spread in; only the link's own entry follows it. + const linkAnchor = + route.params.path === "" && + (route.params.commit ?? null) === pullRequestLinkCommit(route.params.navigationUrl) + ? parsePullRequestAnchor(route.params.navigationUrl) + : null; + const linkPath = + linkAnchor?.kind === "diff" && result.parsed.kind === "files" + ? result.parsed.files.find((file) => pullRequestDiffPathMatches(linkAnchor.digest, file.path)) + ?.path + : undefined; const filter = usePullRequestFileFilter( result.parsed, - route.params.focus ? route.params.path : undefined, + linkPath ?? (route.params.focus ? route.params.path : undefined), ); const { refresh: refreshDetail } = detail; useFocusEffect(refreshDetail); @@ -922,15 +1058,30 @@ function PullRequestFileRouteContent({ route }: PullRequestFileRouteProps) { code: row.content ?? "", }); }; - const [jumpPath, setJumpPath] = useState(route.params.path); + const [pickedPath, setJumpPath] = useState(null); + const jumpPath = pickedPath ?? linkPath ?? route.params.path; const requestedFile = result.parsed.kind === "files" ? result.parsed.files.find((file) => file.path === jumpPath) : undefined; const targetFile = data.files.find((file) => file.path === jumpPath) ?? - (!jumpPath || requestedFile ? data.files[0] : undefined); - const focus = route.params.focus; + (linkAnchor?.kind === "diff" && !linkPath + ? undefined + : !jumpPath || requestedFile + ? data.files[0] + : undefined); + const range = pickedPath === null && linkAnchor?.kind === "diff" ? linkAnchor.range : null; + const focus = + route.params.focus ?? + (range + ? { + side: range.side === "left" ? ("old" as const) : ("new" as const), + start: range.start, + end: range.end, + endSide: range.endSide === "left" ? ("old" as const) : ("new" as const), + } + : undefined); const focusedRows = guideFocusedRows(data.rows, targetFile?.id, focus); const focusIndex = focusedRows.length ? data.rows.findIndex((row) => row.id === focusedRows[0]?.id) @@ -941,7 +1092,7 @@ function PullRequestFileRouteContent({ route }: PullRequestFileRouteProps) { // The filter the current jump was made under. Hiding or showing test files replaces the rows, // so a jump made under the other filter has to run again. Native applies the entry // destination before its first viewport, so the first one counts as made. - const jumpedUnder = useRef(filter.hideTestFiles); + const jumpedUnder = useRef(targetFile ? filter.hideTestFiles : null); const hasTestFiles = result.parsed.kind === "files" && result.parsed.files.some((file) => isTestChangedFile(file.path)); diff --git a/apps/mobile/src/features/pull-requests/PullRequestStatusRows.tsx b/apps/mobile/src/features/pull-requests/PullRequestStatusRows.tsx index 52d5505c45604..03ba26f2247cf 100644 --- a/apps/mobile/src/features/pull-requests/PullRequestStatusRows.tsx +++ b/apps/mobile/src/features/pull-requests/PullRequestStatusRows.tsx @@ -1,3 +1,4 @@ +import { parsePullRequestAnchor } from "@t3tools/shared/pullRequestAnchor"; import { usePullRequestConnected } from "./usePullRequestConnected"; import { useAtomValue } from "@effect/atom-react"; import { useNavigation } from "@react-navigation/native"; @@ -102,7 +103,13 @@ export function PullRequestStatusMenu(props: RowProps) { } export function PullRequestChecksRow(props: RowProps) { - const [expanded, setExpanded] = useState(false); + const linked = parsePullRequestAnchor(props.params.navigationUrl)?.kind === "checks"; + const [expanded, setExpanded] = useState(linked); + const [seenNavigation, setSeenNavigation] = useState(props.params.navigationId); + if (props.params.navigationId !== seenNavigation) { + setSeenNavigation(props.params.navigationId); + if (linked) setExpanded(true); + } const checks = pullRequestChecksSummary(props.pr); const action = usePullRequestAction(props.params, props.pr); const definition = sourceControlClients.get(props.pr.provider); diff --git a/apps/mobile/src/features/pull-requests/PullRequestTimeline.tsx b/apps/mobile/src/features/pull-requests/PullRequestTimeline.tsx index e9f288ea9c60c..f3a491f2ef059 100644 --- a/apps/mobile/src/features/pull-requests/PullRequestTimeline.tsx +++ b/apps/mobile/src/features/pull-requests/PullRequestTimeline.tsx @@ -1,3 +1,7 @@ +import { + parsePullRequestAnchor, + pullRequestAnchorMatchesUrl, +} from "@t3tools/shared/pullRequestAnchor"; import { PullRequestEntryRow } from "./PullRequestEntryRow"; import Svg, { Defs, LinearGradient, Rect, Stop } from "react-native-svg"; import { useAppearancePreferences } from "../settings/appearance/AppearancePreferencesProvider"; @@ -74,7 +78,10 @@ function TimelineComment({ params: PullRequestParams; actions: CommentActions; }) { - const [expanded, setExpanded] = useState(false); + const target = parsePullRequestAnchor(params.navigationUrl); + const linked = + target?.kind === "timeline" && pullRequestAnchorMatchesUrl(target.anchor, comment.url); + const [expanded, setExpanded] = useState(linked); const { themeVariables: colors } = useAppearancePreferences(); const long = comment.body.length > 450 || comment.body.split("\n").length > 8; const content = useMemo( @@ -108,7 +115,13 @@ function TimelineComment({ ) : null} {preview.trim() || content.attachments.length ? ( - + , "navigate" | "dispatch">, actions: CommentActions, ) { + const linkTarget = parsePullRequestAnchor(params.navigationUrl); const discussionIds = new Set( activity.reviewThreads.flatMap((thread) => thread.comments.map((comment) => comment.id)), ); @@ -211,7 +225,19 @@ export function pullRequestTimelineRows( .map((comment) => ({ key: `comment:${comment.id}`, date: comment.createdAt, - content: , + content: ( + + ), })), ...activity.reviewThreads.map((thread) => ({ key: `discussion:${thread.id}`, diff --git a/apps/mobile/src/features/pull-requests/guide/guideDiffFocus.test.ts b/apps/mobile/src/features/pull-requests/guide/guideDiffFocus.test.ts new file mode 100644 index 0000000000000..49f09276d3659 --- /dev/null +++ b/apps/mobile/src/features/pull-requests/guide/guideDiffFocus.test.ts @@ -0,0 +1,30 @@ +import { expect, it } from "vite-plus/test"; +import type { NativeReviewDiffRow } from "../../diffs/nativeReviewDiffSurface"; +import { guideFocusedRows } from "./guideDiffFocus"; + +it("focuses a range that crosses diff sides in either direction", () => { + const rows: NativeReviewDiffRow[] = [ + { kind: "file", id: "file", fileId: "f" }, + { kind: "line", id: "a", fileId: "f", oldLineNumber: 12 }, + { kind: "line", id: "b", fileId: "f", oldLineNumber: 13 }, + { kind: "line", id: "c", fileId: "f", newLineNumber: 12 }, + { kind: "line", id: "d", fileId: "f", newLineNumber: 13 }, + { kind: "line", id: "other", fileId: "other", newLineNumber: 13 }, + ]; + expect( + guideFocusedRows(rows, "f", { side: "old", start: 12, endSide: "new", end: 13 }).map( + (row) => row.id, + ), + ).toEqual(["a", "b", "c", "d"]); + expect( + guideFocusedRows(rows, "f", { side: "new", start: 13, endSide: "old", end: 12 }).map( + (row) => row.id, + ), + ).toEqual(["a", "b", "c", "d"]); + expect( + guideFocusedRows(rows, "f", { side: "old", start: 12, end: 13 }).map((row) => row.id), + ).toEqual(["a", "b"]); + expect(guideFocusedRows(rows, "f", { side: "old", start: 99, endSide: "new", end: 13 })).toEqual( + [], + ); +}); diff --git a/apps/mobile/src/features/pull-requests/guide/guideDiffFocus.ts b/apps/mobile/src/features/pull-requests/guide/guideDiffFocus.ts index cc03bdbb60411..d840efb041f74 100644 --- a/apps/mobile/src/features/pull-requests/guide/guideDiffFocus.ts +++ b/apps/mobile/src/features/pull-requests/guide/guideDiffFocus.ts @@ -2,6 +2,7 @@ import type { NativeReviewDiffRow } from "../../diffs/nativeReviewDiffSurface"; export type GuideDiffFocus = { readonly side: "old" | "new"; + readonly endSide?: "old" | "new"; readonly start: number; readonly end: number; }; @@ -12,6 +13,23 @@ export function guideFocusedRows( focus: GuideDiffFocus | undefined, ) { if (!fileId || !focus) return []; + if (focus.endSide && focus.endSide !== focus.side) { + const start = rows.findIndex( + (row) => + row.fileId === fileId && + (focus.side === "old" ? row.oldLineNumber : row.newLineNumber) === focus.start, + ); + const end = rows.findIndex( + (row) => + row.fileId === fileId && + (focus.endSide === "old" ? row.oldLineNumber : row.newLineNumber) === focus.end, + ); + return start < 0 || end < 0 + ? [] + : rows + .slice(Math.min(start, end), Math.max(start, end) + 1) + .filter((row) => row.fileId === fileId && row.kind === "line"); + } return rows.filter((row) => { const line = focus.side === "old" ? row.oldLineNumber : row.newLineNumber; return ( diff --git a/apps/mobile/src/features/pull-requests/pullRequestNavigation.test.ts b/apps/mobile/src/features/pull-requests/pullRequestNavigation.test.ts index 8a97deffaf06a..8a95397cb3d3a 100644 --- a/apps/mobile/src/features/pull-requests/pullRequestNavigation.test.ts +++ b/apps/mobile/src/features/pull-requests/pullRequestNavigation.test.ts @@ -34,7 +34,13 @@ describe("native PR entry points", () => { context, projects, ), - ).toEqual({ ...context, host: "github.com", repository: "org/repo", number: 12 }); + ).toEqual({ + ...context, + host: "github.com", + repository: "org/repo", + number: 12, + navigationUrl: "https://github.com/org/repo/pull/12/files#diff", + }); expect( nativePullRequestDestination( "https://git.example/group/sub/repo/-/merge_requests/9", diff --git a/apps/mobile/src/features/pull-requests/usePullRequestDiff.ts b/apps/mobile/src/features/pull-requests/usePullRequestDiff.ts index 6f5180d8ab6bc..6e49993a137f3 100644 --- a/apps/mobile/src/features/pull-requests/usePullRequestDiff.ts +++ b/apps/mobile/src/features/pull-requests/usePullRequestDiff.ts @@ -23,6 +23,8 @@ export type PullRequestParams = { readonly repository: string; readonly number: number; readonly host?: string; + readonly navigationUrl?: string; + readonly navigationId?: number; readonly initialSummary?: PullRequestListEntry | PullRequestSummary; }; diff --git a/apps/mobile/src/lib/pullRequestNavigation.ts b/apps/mobile/src/lib/pullRequestNavigation.ts index ecc25375b37d4..40c593b3cab1c 100644 --- a/apps/mobile/src/lib/pullRequestNavigation.ts +++ b/apps/mobile/src/lib/pullRequestNavigation.ts @@ -1,3 +1,4 @@ +import { parsePullRequestAnchor } from "@t3tools/shared/pullRequestAnchor"; import { resolveChangeRequestLinkTarget } from "@t3tools/client-runtime/change-request-links"; import type { EnvironmentProject } from "@t3tools/client-runtime/state/shell"; import { EnvironmentId, ProjectId } from "@t3tools/contracts"; @@ -24,6 +25,7 @@ export function nativePullRequestDestination( projectId: String(target.project.id), repository: target.repository, number: target.number, + ...(parsePullRequestAnchor(url) ? { navigationUrl: url } : {}), ...(target.host === undefined ? {} : { host: target.host }), }; } diff --git a/apps/mobile/src/native/useOpenPullRequest.ts b/apps/mobile/src/native/useOpenPullRequest.ts index ca6edda06a3bc..6608b7b5493db 100644 --- a/apps/mobile/src/native/useOpenPullRequest.ts +++ b/apps/mobile/src/native/useOpenPullRequest.ts @@ -9,6 +9,9 @@ import { appAtomRegistry } from "../state/atom-registry"; import { environmentServerConfigsAtom } from "../state/server"; import { withKnownPullRequestSummary } from "../state/use-thread-pr"; +let linkNavigationId = 0; +const nextLinkNavigationId = () => ++linkNavigationId; + type PullRequestContext = { readonly environmentId: string; readonly projectId: string }; // Avoid useRoute so layouts without route context can open any thread's pull requests. @@ -24,7 +27,10 @@ export function useNavigateToPullRequest(options?: { readonly replace?: boolean serverConfigs.has(environmentId), ); if (!destination) return tryOpenExternalUrl(url, "pull-request"); - const params = withKnownPullRequestSummary(destination); + const params = withKnownPullRequestSummary({ + ...destination, + ...(destination.navigationUrl ? { navigationId: nextLinkNavigationId() } : {}), + }); // Android shows one page at a time; iOS opens it over the current page, so Back returns. if (Platform.OS === "android") selectDetail({ name: "PullRequest", params }); else if (replace) navigation.dispatch(StackActions.replace("PullRequest", params)); diff --git a/apps/web/src/components/chat/RightPanelSurfaceContent.tsx b/apps/web/src/components/chat/RightPanelSurfaceContent.tsx index f0d8afa5f319c..5ba6985e19d21 100644 --- a/apps/web/src/components/chat/RightPanelSurfaceContent.tsx +++ b/apps/web/src/components/chat/RightPanelSurfaceContent.tsx @@ -150,6 +150,10 @@ export function RightPanelSurfaceContent(props: { number: surface.number, }} initialTab={surface.detailTab ?? "summary"} + revealUrl={surface.revealUrl} + onRevealSettled={(revealUrl) => { + useRightPanelStore.getState().settlePullRequestReveal(threadRef, surface.id, revealUrl); + }} onTabChange={(tab) => { useRightPanelStore.getState().setPullRequestDetailTab(threadRef, surface.id, tab); }} diff --git a/apps/web/src/components/diffs/useCodeViewFileReveal.test.ts b/apps/web/src/components/diffs/useCodeViewFileReveal.test.ts index c153120d2b481..d82c09ef2aa53 100644 --- a/apps/web/src/components/diffs/useCodeViewFileReveal.test.ts +++ b/apps/web/src/components/diffs/useCodeViewFileReveal.test.ts @@ -6,11 +6,13 @@ import { createCodeViewFileReveal } from "./useCodeViewFileReveal"; function viewer(initialItems: ReadonlyArray) { const items = new Set(initialItems); const scrolls: CodeViewScrollTarget[] = []; + const collapsed = new Set(); return { items, + collapsed, scrolls, getInstance: () => ({}), - getItem: (id: string) => (items.has(id) ? {} : undefined), + getItem: (id: string) => (items.has(id) ? { collapsed: collapsed.has(id) } : undefined), scrollTo: (target: CodeViewScrollTarget) => scrolls.push(target), }; } @@ -24,6 +26,13 @@ describe("createCodeViewFileReveal", () => { reveal.request("a"); expect(view.scrolls).toEqual([{ type: "item", id: "a", align: "start" }]); + const range = { start: 12, side: "deletions", end: 15, endSide: "additions" } as const; + view.collapsed.add("a"); + reveal.request("a", range); + expect(view.scrolls).toHaveLength(1); + view.collapsed.delete("a"); + reveal.flush(); + expect(view.scrolls.at(-1)).toEqual({ type: "range", id: "a", range, align: "center" }); }); it("keeps a missing file pending until a flush finds it, then drops it", () => { @@ -54,19 +63,126 @@ describe("createCodeViewFileReveal", () => { expect(view.scrolls).toEqual([{ type: "item", id: "a", align: "start" }]); }); - it("drops a pending reveal when its viewer detaches, and positions the next viewer on request", () => { + it("renders the destination file and expands context before positioning its range", () => { + const reveal = createCodeViewFileReveal(); + const view = viewer(["a"]); + let rendered = false; + let expanded = false; + const instance = { + revealLine: () => { + if (expanded) return false; + expanded = true; + return true; + }, + getLinePosition: () => (expanded ? { top: 100, height: 20 } : undefined), + }; + reveal.attach({ + ...view, + getInstance: () => ({ getRenderedItems: () => (rendered ? [{ id: "a", instance }] : []) }), + }); + const range = { start: 12, end: 15, side: "additions" } as const; + reveal.request("a", range); + expect(view.scrolls).toEqual([{ type: "item", id: "a", align: "start" }]); + rendered = true; + reveal.flush(); + expect(expanded).toBe(true); + expect(view.scrolls).toHaveLength(1); + reveal.flush(); + expect(view.scrolls.at(-1)).toEqual({ type: "range", id: "a", range, align: "center" }); + }); + + it("expands a patch-only file to an unchanged line before positioning it", () => { + const reveal = createCodeViewFileReveal(); + const view = viewer(["a"]); + let partial = true; + const expanded: unknown[] = []; + const instance = { + getDiffForSearch: () => + ({ + isPartial: partial, + hunks: [{ additionStart: 10, additionCount: 5, deletionStart: 10, deletionCount: 5 }], + }) as never, + expandHunk: (...args: unknown[]) => expanded.push(args), + revealLine: () => false, + getLinePosition: () => ({ top: 100, height: 20 }), + }; + reveal.attach({ + ...view, + getInstance: () => ({ getRenderedItems: () => [{ id: "a", instance }] }), + }); + const range = { start: 30, end: 30, side: "additions" } as const; + reveal.request("a", range); + expect(expanded).toEqual([[1, "up", 26]]); + reveal.flush(); + expect(view.scrolls).toEqual([]); + // The expander loads the file; the next render holds the line. + partial = false; + reveal.flush(); + expect(view.scrolls).toEqual([{ type: "range", id: "a", range, align: "center" }]); + + // Removed lines always sit in a hunk, and a deleted file has no contents to load. + const deleted = viewer(["d"]); + const removedRange = { start: 3, end: 4, side: "deletions" } as const; + reveal.attach({ + ...deleted, + getInstance: () => ({ + getRenderedItems: () => [ + { + id: "d", + instance: { + ...instance, + getDiffForSearch: () => + ({ + isPartial: true, + hunks: [ + { additionStart: 0, additionCount: 0, deletionStart: 1, deletionCount: 8 }, + ], + }) as never, + }, + }, + ], + }), + }); + reveal.request("d", removedRange); + expect(expanded).toHaveLength(1); + expect(deleted.scrolls).toEqual([ + { type: "range", id: "d", range: removedRange, align: "center" }, + ]); + }); + + it("drops a pending reveal when another diff's viewer replaces it, but repositions a range a remount loses", () => { const reveal = createCodeViewFileReveal(); const first = viewer([]); - const second = viewer(["a"]); + // Item ids carry the diff's scope, so another diff's viewer holds other ids. + const second = viewer(["b"]); reveal.attach(first); reveal.request("a"); reveal.attach(null); reveal.attach(second); + second.items.add("a"); reveal.flush(); expect(second.scrolls).toEqual([]); reveal.request("a"); expect(second.scrolls).toHaveLength(1); + + // The first diff load remounts the viewer right after a range scroll, so it is repeated once. + const range = { start: 3, end: 4, side: "additions" } as const; + const remounted = viewer(["a"]); + reveal.request("a", range); + reveal.attach(null); + reveal.attach(remounted); + reveal.flush(); + expect(remounted.scrolls).toEqual([{ type: "range", id: "a", range, align: "center" }]); + + // Scrolling away before that second pass keeps the user where they went. + const scrolledAway = viewer(["a"]); + reveal.request("a", range); + reveal.cancel(); + reveal.attach(null); + reveal.attach(scrolledAway); + reveal.flush(); + expect(scrolledAway.scrolls).toEqual([]); }); }); diff --git a/apps/web/src/components/diffs/useCodeViewFileReveal.ts b/apps/web/src/components/diffs/useCodeViewFileReveal.ts index f0779262dd1ac..60b76290fbe2f 100644 --- a/apps/web/src/components/diffs/useCodeViewFileReveal.ts +++ b/apps/web/src/components/diffs/useCodeViewFileReveal.ts @@ -1,37 +1,176 @@ -import type { CodeViewScrollTarget } from "@pierre/diffs"; +import type { + CodeViewScrollTarget, + SelectedLineRange, + FileDiffMetadata, + SelectionSide, +} from "@pierre/diffs"; import { useState } from "react"; interface FileRevealHandle { - getInstance(): object | undefined; - getItem(id: string): object | undefined; + getInstance(): + | { + getRenderedItems?(): readonly { + id: string; + instance: { + getDiffForSearch?(): FileDiffMetadata | undefined; + revealLine?(line: number): boolean; + expandHunk?(hunkIndex: number, direction: "up" | "down", lineCount: number): void; + getLinePosition( + line: number, + side?: SelectionSide, + ): { top: number; height: number } | undefined; + }; + }[]; + } + | undefined; + getItem(id: string): { collapsed?: boolean } | undefined; scrollTo(target: CodeViewScrollTarget): void; } +// Mirrors the viewer's `revealLine`, which reads distances from the edges of the collapsed gap. +function expandPartialDiffToLine( + diff: FileDiffMetadata, + line: number, + expandHunk: (hunkIndex: number, direction: "up" | "down", lineCount: number) => void, +): boolean { + const { hunks } = diff; + if ( + hunks.some( + (hunk) => line >= hunk.additionStart && line < hunk.additionStart + hunk.additionCount, + ) + ) { + return false; + } + const next = hunks.findIndex((hunk) => line < hunk.additionStart); + const previous = next === -1 ? hunks.at(-1) : hunks[next - 1]; + const fromStart = line - (previous ? previous.additionStart + previous.additionCount : 1) + 1; + const after = next === -1 ? undefined : hunks[next]; + if (after === undefined) expandHunk(hunks.length, "up", fromStart + REVEAL_CONTEXT_LINES); + else if (fromStart <= after.additionStart - line) { + expandHunk(next, "up", fromStart + REVEAL_CONTEXT_LINES); + } else expandHunk(next, "down", after.additionStart - line + REVEAL_CONTEXT_LINES); + return true; +} + +const REVEAL_CONTEXT_LINES = 10; + /** * Scrolls a code view to a file. `attach` is the viewer's ref and `flush` its post-render * handler, so a reveal asked before the viewer mounts, or for a file it does not hold yet, is - * positioned once it does. A reveal belongs to the diff it was asked of, so detaching the viewer, - * such as remounting it for another diff scope, drops it. + * positioned once it does. A reveal belongs to the diff it was asked of, so a viewer remounted for + * another diff scope, which no longer holds the file, drops it. */ export function createCodeViewFileReveal() { let viewer: FileRevealHandle | null = null; - let pending: string | null = null; + let detachedInstance: object | undefined; + let pending: { + id: string; + range?: SelectedLineRange; + positionedFile?: boolean; + loadingFile?: boolean; + settling?: boolean; + } | null = null; const flush = () => { if (pending === null || viewer === null) return; - if (!viewer.getInstance() || viewer.getItem(pending) === undefined) return; - viewer.scrollTo({ type: "item", id: pending, align: "start" }); - pending = null; + const item = viewer.getItem(pending.id); + if (!viewer.getInstance() || item === undefined || item.collapsed === true) return; + const renderedItems = viewer.getInstance()?.getRenderedItems?.(); + if (pending.range && renderedItems) { + const rendered = renderedItems.find((entry) => entry.id === pending?.id); + if (!rendered) { + if (!pending.positionedFile) { + pending.positionedFile = true; + viewer.scrollTo({ type: "item", id: pending.id, align: "start" }); + } + return; + } + const diff = rendered.instance.getDiffForSearch?.(); + const additionLine = (line: number, side: SelectionSide | undefined) => { + if (side !== "deletions" || !diff) return line; + const next = diff.hunks.find((hunk) => line < hunk.deletionStart + hunk.deletionCount); + const last = diff.hunks.at(-1); + return next + ? line + next.additionStart - next.deletionStart + : last + ? line + + last.additionStart + + last.additionCount - + last.deletionStart - + last.deletionCount + : line; + }; + const { range } = pending; + const startLine = additionLine(range.start, range.side); + const endLine = additionLine(range.end, range.endSide ?? range.side); + // A patch-only diff holds no unchanged lines, and `revealLine` declines to expand it. The + // viewer's own expander loads the file first, so a line outside every hunk is opened with it. + if (diff?.isPartial && rendered.instance.expandHunk) { + // Wait for the file contents the expander asked for. + if (pending.loadingFile) return; + const { expandHunk } = rendered.instance; + // A removed line is always in a hunk, and a deleted file has no contents to load. + const removed = (line: number, side: SelectionSide | undefined) => + side === "deletions" && + diff.hunks.some( + (hunk) => line >= hunk.deletionStart && line < hunk.deletionStart + hunk.deletionCount, + ); + const outside = [ + ...new Set([ + ...(removed(range.start, range.side) ? [] : [startLine]), + ...(removed(range.end, range.endSide ?? range.side) ? [] : [endLine]), + ]), + ]; + const opened = outside.map((line) => expandPartialDiffToLine(diff, line, expandHunk)); + if (opened.some(Boolean)) { + pending.loadingFile = true; + return; + } + } + const startExpanded = rendered.instance.revealLine?.(startLine); + const endExpanded = rendered.instance.revealLine?.(endLine); + if (startExpanded || endExpanded) return; + if ( + !rendered.instance.getLinePosition(range.start, range.side) || + !rendered.instance.getLinePosition(range.end, range.endSide ?? range.side) + ) + return; + } + const target = pending; + // The diff's first load can remount the viewer or re-render it right after this scroll, which + // loses it; the range is positioned once more on the next render that can hold it. + pending = target.range && !target.settling ? { ...target, settling: true } : null; + viewer.scrollTo( + target.range + ? { type: "range", id: target.id, range: target.range, align: "center" } + : { type: "item", id: target.id, align: "start" }, + ); }; return { attach(next: FileRevealHandle | null) { - if (viewer !== null && next !== viewer) pending = null; + // The viewer rebuilds its handle on every render and can remount during the diff's first + // load. Item ids carry the diff's scope, so only a viewer without the item drops the reveal. + const previous = viewer?.getInstance() ?? detachedInstance; + if (next === null) detachedInstance = previous; + else { + const instance = next.getInstance(); + if (pending !== null && previous !== undefined && instance !== previous) { + if (instance === undefined || next.getItem(pending.id) === undefined) pending = null; + // A new instance starts from the patch again, so it must expand the line itself. + else pending.loadingFile = false; + } + detachedInstance = undefined; + } viewer = next; flush(); }, - request(fileKey: string) { - pending = fileKey; + request(fileKey: string, range?: SelectedLineRange) { + pending = { id: fileKey, ...(range ? { range } : {}) }; flush(); }, + /** The user scrolled; a reveal still being positioned must not pull them back. */ + cancel() { + pending = null; + }, flush, }; } diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx index cea33e74ffc99..96ef692c4aa13 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx @@ -1,3 +1,5 @@ +import { sha256 } from "@noble/hashes/sha2"; +import { bytesToHex } from "@noble/hashes/utils"; import { EnvironmentId, ProjectId, @@ -8,6 +10,8 @@ import { import { expect, it, vi } from "vite-plus/test"; import "~/index.css"; +import { __setClientSettingsForTests } from "../../hooks/useSettings"; +import { DEFAULT_CLIENT_SETTINGS } from "@t3tools/contracts/settings"; const { diffAtom, pagedDiffAtom } = await vi.hoisted(async () => { const { AsyncResult, Atom } = await import("effect/reactivity"); @@ -34,11 +38,13 @@ const { diffAtom, pagedDiffAtom } = await vi.hoisted(async () => { }; const pages: Record = { first: { - patch: Array.from({ length: 40 }, (_, index) => filePatch(`early-${index}.ts`)).join(""), + patch: Array.from({ length: 40 }, (_, index) => + filePatch(index === 1 ? "early-1.test.ts" : `early-${index}.ts`), + ).join(""), truncated: true, nextCursor: "page-2", }, - "page-2": { patch: filePatch("late.ts"), truncated: false, nextCursor: null }, + "page-2": { patch: filePatch("late.test.ts"), truncated: false, nextCursor: null }, }; return { // An environment query whose refetch returns equal content hands back the previous value @@ -126,6 +132,8 @@ const detail: PullRequestDetailView = { }; function setup({ paged }: { paged: boolean }) { + window.localStorage.removeItem("t3code.diffHideTestFiles"); + __setClientSettingsForTests(DEFAULT_CLIENT_SETTINGS); mocks.paged = paged; mocks.diffCommits = []; } @@ -133,6 +141,8 @@ function setup({ paged }: { paged: boolean }) { const tab = ( refreshToken: number, revealRequest: { readonly path: string; readonly nonce: number } | null = null, + revealUrl?: string, + onRevealSettled?: (url: string) => void, ) => ( {}} @@ -144,6 +154,8 @@ const tab = ( onRefresh={() => {}} refreshToken={refreshToken} revealRequest={revealRequest} + revealUrl={revealUrl} + onRevealSettled={onRevealSettled} /> ); @@ -193,13 +205,30 @@ it("scrolls to a file asked for as the tab opens", async () => { it("pages forward to a file asked for that is not loaded yet, then scrolls to it", async () => { setup({ paged: true }); + __setClientSettingsForTests({ ...DEFAULT_CLIENT_SETTINGS, diffFilesCollapsed: true }); + using _preferences = { + [Symbol.dispose]() { + window.localStorage.removeItem("t3code.diffHideTestFiles"); + __setClientSettingsForTests(DEFAULT_CLIENT_SETTINGS); + }, + }; const container = pagedContainer(); await using screen = await render(tab(0), { container }); await expect.element(screen.getByText("early-0.ts", { exact: true }).first()).toBeVisible(); + await screen.getByRole("button", { name: "Hide test files" }).click(); - await screen.rerender(tab(0, { path: "late.ts", nonce: 1 })); + const url = `${detail.url}/files#diff-${bytesToHex(sha256(new TextEncoder().encode("late.test.ts")))}R4-R5`; + const settled = vi.fn(); + await screen.rerender(tab(0, null, url, settled)); - await expectFileHeaderShown(screen, container, "late.ts"); + await expectFileHeaderShown(screen, container, "late.test.ts"); + await expect.element(screen.getByText("three()", { exact: true })).toBeVisible(); + await vi.waitFor(() => expect(settled).toHaveBeenCalledWith(url)); + // A settled link keeps its test file past the filter, and a later click reveals it again. + await screen.rerender(tab(0, { path: "early-0.ts", nonce: 1 })); + await expectFileHeaderShown(screen, container, "early-0.ts"); + await screen.rerender(tab(0, { path: "early-0.ts", nonce: 1 }, url, settled)); + await expectFileHeaderShown(screen, container, "late.test.ts"); }); it("never requests the diff of a selected commit the pull request no longer has", async () => { @@ -235,3 +264,37 @@ it("never requests the diff of a selected commit the pull request no longer has" .toBeVisible(); expect(mocks.diffCommits).not.toContain("gone"); }); + +it("answers a commit's diff link only once that commit's diff is shown", async () => { + setup({ paged: false }); + const container = document.createElement("div"); + container.className = "flex h-[700px] w-[900px]"; + const oid = "c".repeat(40); + const withCommit = { + ...detail, + commits: [{ oid, messageHeadline: "Linked commit", committedDate: "2026-09-01T00:00:00Z" }], + }; + const url = `${detail.url}/commits/${oid}#diff-${bytesToHex(sha256(new TextEncoder().encode("a.ts")))}R2`; + const settled = vi.fn(); + const scoped = (selectedCommitOid: string | null, revealUrl?: string) => ( + {}} + environmentId={EnvironmentId.make("environment")} + reference={detail} + detail={withCommit} + selectedCommitOid={selectedCommitOid} + onSelectedCommitChange={() => {}} + onRefresh={() => {}} + revealUrl={revealUrl} + onRevealSettled={settled} + /> + ); + await using screen = await render(scoped(null), { container }); + await expect.element(screen.getByText("1 file", { exact: true })).toBeVisible(); + + // The panel selects the link's commit after this tab has already seen the link. + await screen.rerender(scoped(null, url)); + expect(settled).not.toHaveBeenCalled(); + await screen.rerender(scoped(oid, url)); + await vi.waitFor(() => expect(settled).toHaveBeenCalledWith(url)); +}); diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 31fd309348be7..88fe865e917a5 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -1,3 +1,8 @@ +import { + parsePullRequestAnchor, + pullRequestDiffPathMatches, + pullRequestLinkCommit, +} from "@t3tools/shared/pullRequestAnchor"; import { capitalizeTerm, sourceControlClients, @@ -348,6 +353,8 @@ function PullRequestCodeTab({ refreshToken = 0, backgroundRefreshToken = 0, revealRequest = null, + revealUrl, + onRevealSettled, }: { scrollerRef: (node: HTMLDivElement | null) => void; environmentId: EnvironmentId; @@ -364,6 +371,9 @@ function PullRequestCodeTab({ onRefresh: () => void; refreshToken?: number; backgroundRefreshToken?: number; + /** A clicked link to a file or line range, until onRevealSettled reports it shown. */ + revealUrl?: string | undefined; + onRevealSettled?: ((revealUrl: string) => void) | undefined; revealRequest?: { readonly path: string; readonly nonce: number } | null; }) { const resolvedTheme = useRenderedAppearance(); @@ -575,6 +585,20 @@ function PullRequestCodeTab({ `pull-request:${scopeKey}:${resolvedTheme}:${ignoreWhitespace}`, ignoreWhitespace, ); + const linkAnchor = parsePullRequestAnchor(revealUrl); + const linkFile = + linkAnchor?.kind === "diff" + ? loadedFiles.find((file) => + pullRequestDiffPathMatches(linkAnchor.digest, resolveFileDiffPath(file)), + ) + : undefined; + const pendingLinkPath = linkFile ? resolveFileDiffPath(linkFile) : null; + // The linked file stays shown past the test-file filter after the link settles. + const [revealedLink, setRevealedLink] = useState<{ + readonly scope: string; + readonly path: string; + } | null>(null); + const linkPath = pendingLinkPath ?? (revealedLink?.scope === scopeKey ? revealedLink.path : null); const testFileCount = loadedFiles.filter((file) => isTestChangedFile(resolveFileDiffPath(file)), ).length; @@ -582,7 +606,7 @@ function PullRequestCodeTab({ const files = hideTestFiles ? loadedFiles.filter((file) => { const path = resolveFileDiffPath(file); - return path === revealRequest?.path || !isTestChangedFile(path); + return path === revealRequest?.path || path === linkPath || !isTestChangedFile(path); }) : loadedFiles; const hiddenTestFileCount = loadedFiles.length - files.length; @@ -790,7 +814,7 @@ function PullRequestCodeTab({ setFileViewed, }; - const revealFile = (path: string) => { + const revealFile = (path: string, range?: SelectedLineRange) => { const item = items.find((candidate) => resolveFileDiffPath(candidate.fileDiff) === path); if (item === undefined) return; // Unfold only: a second reveal before the unfold renders must not fold the file again. @@ -799,7 +823,8 @@ function PullRequestCodeTab({ toggleFileDiffFoldForViewed(item.id, false, effectiveFoldOverride, current), ); } - codeViewReveal.request(item.id); + if (range) setSelectedLines({ id: item.id, range }); + codeViewReveal.request(item.id, range); }; const toggleAllFiles = () => { @@ -1430,6 +1455,53 @@ function PullRequestCodeTab({ answerReveal(); // oxlint-disable-next-line react/exhaustive-effect-dependencies -- Re-checked as pages load until the requested file arrives. }, [canLoadNextSlice, items, revealRequest]); + // The store clears a settled link after this render, so one link must not reveal twice. + const answeredLink = useRef(null); + const revealLink = useEffectEvent(() => { + if (revealUrl === undefined) answeredLink.current = null; + if (revealUrl === undefined || linkAnchor?.kind !== "diff") return; + // The panel selects the link's commit after this tab's effects run, so the files on screen + // can still be another commit's. + const linkCommit = pullRequestLinkCommit(revealUrl); + if ((detail.commits.some((entry) => entry.oid === linkCommit) ? linkCommit : null) !== commit) { + return; + } + const key = `${scopeKey}:${revealUrl}`; + if (answeredLink.current === key) return; + if ( + !pendingLinkPath || + !items.some((item) => resolveFileDiffPath(item.fileDiff) === pendingLinkPath) + ) { + if (canLoadNextSlice) loadNextSlice(); + // Every page is loaded and none has the file. + else if (!pendingLinkPath && loadedSlices.length > 0 && nextCursor === null) { + answeredLink.current = key; + onRevealSettled?.(revealUrl); + } + return; + } + answeredLink.current = key; + setRevealedLink({ scope: scopeKey, path: pendingLinkPath }); + onRevealSettled?.(revealUrl); + const range = linkAnchor.range; + revealFile( + pendingLinkPath, + range + ? { + start: range.start, + end: range.end, + side: toViewerSide(range.side), + endSide: toViewerSide(range.endSide), + } + : undefined, + ); + }); + // oxlint-disable-next-line t3code/no-unreasoned-state-sync -- pages and scrolls the virtual diff to a clicked file or line range + useEffect(() => { + // oxlint-disable-next-line react/set-state-in-effect -- revealing unfolds the target and may request the next diff page + revealLink(); + // oxlint-disable-next-line react/exhaustive-effect-dependencies -- retries the reveal as diff pages and viewer items arrive + }, [revealUrl, pendingLinkPath, canLoadNextSlice, items]); const withToolbar = (body: ReactNode) => (
{toolbar} @@ -1534,6 +1606,10 @@ function PullRequestCodeTab({
{ const composedPath = event.nativeEvent.composedPath?.() ?? []; diff --git a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx index 9d0f6193f7e3f..81dea810bb0d0 100644 --- a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx @@ -1,3 +1,9 @@ +import { + parsePullRequestAnchor, + pullRequestLinkCommit, + resolvePullRequestReviewAnchor, + type PullRequestAnchor, +} from "@t3tools/shared/pullRequestAnchor"; import { projectChangeRequestUrl } from "@t3tools/client-runtime/change-request-links"; import { normalizeThreadPullRequestKey } from "@t3tools/client-runtime/thread-pull-requests"; import { useAtomValue } from "@effect/atom-react"; @@ -126,6 +132,8 @@ import { isStackedPullRequestBase, pullRequestActionNeedsHostRefresh, pullRequestCheckoutCommand, + pullRequestTimelineEventMatches, + buildPullRequestTimeline, pullRequestFindingKey, pullRequestHandoffLabels, PULL_REQUEST_MERGE_METHOD_LABELS, @@ -255,6 +263,22 @@ function pullRequestStackReference( }; } +function linkedTab(anchor: PullRequestAnchor | null): PullRequestDetailTab | null { + switch (anchor?.kind) { + case "code": + case "diff": + return "code"; + case "commits": + case "timeline": + return "timeline"; + case "checks": + case "summary": + return "summary"; + default: + return null; + } +} + export function PullRequestDetailPanel({ environmentId, shortcutsEnabled, @@ -271,6 +295,8 @@ export function PullRequestDetailPanel({ onBack, onSelectPullRequest, onGuideShown, + revealUrl, + onRevealSettled, initialTab = "summary", onTabChange, }: { @@ -297,6 +323,10 @@ export function PullRequestDetailPanel({ composerDraftTarget?: ScopedThreadRef | DraftId; onBack?: (() => void) | undefined; onGuideShown?: (() => void) | undefined; + /** A clicked link into this pull request, waiting to be shown. */ + revealUrl?: string | undefined; + /** Reports the link it was given shown or missing, so a newer link stays pending. */ + onRevealSettled?: ((revealUrl: string) => void) | undefined; initialTab?: PullRequestDetailTab; onTabChange?: ((tab: PullRequestDetailTab) => void) | undefined; }) { @@ -355,7 +385,10 @@ export function PullRequestDetailPanel({ coreDetail, detail, } = usePullRequestDetail(environmentId, reference, matchingListEntry); - const [selectedTab, setTab] = useState(initialTab); + const revealAnchor = parsePullRequestAnchor(revealUrl); + const [selectedTab, setTab] = useState( + () => linkedTab(revealAnchor) ?? initialTab, + ); const reviewGuidesEnabled = useEnvironmentSettings( environmentId, (settings) => settings.reviewGuidesEnabled, @@ -379,7 +412,7 @@ export function PullRequestDetailPanel({ const [codeCommitScope, setCodeCommitScope] = useState<{ readonly pullRequestKey: string; readonly oid: string | null; - }>(() => ({ pullRequestKey, oid: null })); + }>(() => ({ pullRequestKey, oid: pullRequestLinkCommit(revealUrl) })); const selectedCodeCommitOid = codeCommitScope.pullRequestKey === pullRequestKey ? codeCommitScope.oid : null; const selectCodeCommit = (oid: string | null) => { @@ -399,6 +432,33 @@ export function PullRequestDetailPanel({ void loadCodeTab(); selectTab("code"); }; + // Cached activity can predate a linked comment, so a miss waits for one host read. `after` is + // when that read's invalidation finished; null while it runs. + const revealHostRead = useRef<{ readonly url: string; readonly after: number | null } | null>( + null, + ); + const settleReveal = (url: string) => { + revealHostRead.current = null; + onRevealSettled?.(url); + }; + const followLink = useEffectEvent(() => { + if (revealUrl === undefined) return; + const next = linkedTab(revealAnchor); + if (next === null) return settleReveal(revealUrl); + if (next === "code") { + selectCodeCommit(pullRequestLinkCommit(revealUrl)); + void loadCodeTab(); + } + selectTab(next); + // These name a tab, not a place in it. + if (revealAnchor?.kind === "code" || revealAnchor?.kind === "commits") settleReveal(revealUrl); + }); + // oxlint-disable-next-line t3code/no-unreasoned-state-sync -- a clicked link lives in the panel store and selects its tab even when the panel is already mounted + useEffect(() => { + // oxlint-disable-next-line react/set-state-in-effect -- link navigation selects the PR tab and commit scope + followLink(); + // oxlint-disable-next-line react/exhaustive-effect-dependencies -- each clicked link is a navigation request + }, [revealUrl]); const tabScopeKey = `${environmentId}:${pullRequestKey}`; const [tabMountState, setTabMountState] = useState(() => ({ key: tabScopeKey, @@ -601,14 +661,72 @@ export function PullRequestDetailPanel({ const invalidate = useAtomCommand(pullRequestEnvironment.invalidate, { reportFailure: false }); const [isInvalidating, setIsInvalidating] = useState(false); const refreshing = isInvalidating || detailQuery.isPending; - const refreshFromHost = async () => { + /** Resolves whether the host cache was invalidated before the refetch. */ + const refetchFromHost = async () => { setIsInvalidating(true); // Atom commands settle without throwing, so reset always runs. - await invalidate({ environmentId, input: { reference } }); + const invalidated = await invalidate({ environmentId, input: { reference } }); refreshDetail(); setRefreshToken((token) => token + 1); setIsInvalidating(false); + return invalidated._tag === "Success"; + }; + const refreshFromHost = async () => { + await refetchFromHost(); }; + const timelineAnchor = + revealAnchor?.kind !== "timeline" + ? null + : detail === null + ? revealAnchor.anchor + : resolvePullRequestReviewAnchor( + revealAnchor.anchor, + detail.comments, + detail.reviewThreads, + ); + // Loaded activity without the linked item, keyed by load so a refetch is checked again. + const revealMissedAt = + timelineAnchor !== null && + detail !== null && + !activityQuery.isPending && + !buildPullRequestTimeline(detail).some((event) => + pullRequestTimelineEventMatches(event, timelineAnchor), + ) + ? activityQuery.dataUpdatedAt + : null; + const revealReadFailed = revealMissedAt !== null && activityQuery.error !== null; + const readHostForReveal = useEffectEvent(() => { + if (revealUrl === undefined || revealMissedAt === null) return; + const read = revealHostRead.current; + if (read?.url !== revealUrl) { + const request = { url: revealUrl, after: null }; + revealHostRead.current = request; + void refetchFromHost().then((invalidated) => { + if (revealHostRead.current !== request) return; + // Without an invalidation the refetch could only repeat the cached read. + if (!invalidated) return settleReveal(request.url); + revealHostRead.current = { url: request.url, after: Date.now() }; + }); + return; + } + // A read that finished before the invalidation cannot show the item is missing. `after` is + // stamped once the refetch was requested, so a read in that same millisecond is the new one. + if (read.after !== null && (revealReadFailed || revealMissedAt >= read.after)) { + settleReveal(revealUrl); + } + }); + // oxlint-disable-next-line t3code/no-unreasoned-state-sync -- a linked comment missing from cached activity asks the host once before the link settles + useEffect(() => { + readHostForReveal(); + // oxlint-disable-next-line react/exhaustive-effect-dependencies -- each load that misses the linked item is checked against the host read + }, [revealMissedAt, revealReadFailed, revealUrl]); + // oxlint-disable-next-line t3code/no-unreasoned-state-sync -- a host read outliving the panel must not settle a later link + useEffect( + () => () => { + revealHostRead.current = null; + }, + [], + ); const refreshFromHostOnRequest = useEffectEvent(() => void refreshFromHost()); const appliedForcedToken = useRef(forcedRefreshToken); // oxlint-disable-next-line t3code/no-unreasoned-state-sync -- a new forced refresh token invalidates the host cache and refetches @@ -1746,6 +1864,8 @@ export function PullRequestDetailPanel({
diff --git a/apps/web/src/components/pullRequest/PullRequestLinkPreview.tsx b/apps/web/src/components/pullRequest/PullRequestLinkPreview.tsx index 8e3ee75602342..4d5668937880e 100644 --- a/apps/web/src/components/pullRequest/PullRequestLinkPreview.tsx +++ b/apps/web/src/components/pullRequest/PullRequestLinkPreview.tsx @@ -1,5 +1,6 @@ import type { PreviewCard as PreviewCardPrimitive } from "@base-ui/react/preview-card"; import { isAtomCommandInterrupted } from "@t3tools/client-runtime/state/runtime"; +import { parsePullRequestAnchor, pullRequestLinkCommit } from "@t3tools/shared/pullRequestAnchor"; import type { EnvironmentId, PullRequestRef } from "@t3tools/contracts"; import { cloneElement, @@ -18,7 +19,12 @@ import { useEnvironmentQuery } from "~/state/query"; import { PreviewCard, PreviewCardPopup, PreviewCardTrigger } from "../ui/preview-card"; import { Tooltip, TooltipPopup, TooltipTrigger } from "../ui/tooltip"; -import { PullRequestActorAvatar, resolvePullRequestState } from "./pullRequestPresentation"; +import { pullRequestLinkSubject, pullRequestReviewOutcome } from "./pullRequestDetail.logic"; +import { + PullRequestActorAvatar, + PullRequestReviewOutcomeBadge, + resolvePullRequestState, +} from "./pullRequestPresentation"; interface PullRequestLinkPreviewTarget { readonly environmentId: EnvironmentId; @@ -57,6 +63,20 @@ export function PullRequestLinkPreview({ }) : null, ); + // A link to a comment or commit shows that, read from the activity the panel shares. + const namesSubject = + parsePullRequestAnchor(originalUrl)?.kind === "timeline" || + pullRequestLinkCommit(originalUrl) !== null; + const activityQuery = useEnvironmentQuery( + open && namesSubject + ? pullRequestEnvironment.activity({ + environmentId: target.environmentId, + input: target.input, + }) + : null, + ); + const subject = + activityQuery.data === null ? null : pullRequestLinkSubject(originalUrl, activityQuery.data); const summary = useSharedPullRequestSummary( open ? target.environmentId : null, open ? target.input : null, @@ -136,10 +156,16 @@ export function PullRequestLinkPreview({ )}
-

- {card.title} -

- {card.author === undefined ? null : ( + {subject?.kind === "comment" ? ( + + ) : subject?.kind === "commit" ? ( + + ) : ( +

+ {card.title} +

+ )} + {card.author === undefined || subject !== null ? null : (
{authorLabel} @@ -161,3 +187,68 @@ export function PullRequestLinkPreview({ ); } + +function actorLabel(actor: { readonly login: string } | null | undefined) { + return actor?.login ?? "ghost"; +} + +function LinkedComment({ + subject, +}: { + subject: Extract, { kind: "comment" }>; +}) { + const outcome = pullRequestReviewOutcome(subject.reviewState); + return ( +
+
+ + + {actorLabel(subject.author)} + + {outcome === null ? ( + commented + ) : ( + + )} + · + + + +
+ {subject.excerpt === null ? null : ( +

+ {subject.excerpt} +

+ )} +
+ ); +} + +function LinkedCommit({ + commit, +}: { + commit: Extract, { kind: "commit" }>["commit"]; +}) { + const author = commit.authors?.[0]; + return ( +
+

+ {commit.messageHeadline} +

+
+ {commit.oid.slice(0, 7)} + {author === undefined ? null : ( + <> + · + + {actorLabel(author)} + + )} + · + + + +
+
+ ); +} diff --git a/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx b/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx index b98a48a817b0e..21a444cd27378 100644 --- a/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx @@ -1,3 +1,5 @@ +import { parsePullRequestAnchor } from "@t3tools/shared/pullRequestAnchor"; +import { holdLinkedNodeInView } from "./holdLinkedNodeInView"; import { useAtomValue } from "@effect/atom-react"; import { capitalizeTerm, @@ -23,7 +25,7 @@ import { UsersIcon, } from "lucide-react"; import * as Schema from "effect/Schema"; -import { useRef, useState, type ReactNode } from "react"; +import { useEffect, useEffectEvent, useRef, useState, type ReactNode } from "react"; import { pullRequestEnvironment } from "~/state/pullRequests"; import { useLocalStorage } from "~/hooks/useLocalStorage"; @@ -356,16 +358,27 @@ function Section({ title, defaultOpen = true, keepMounted = false, + revealCount = 0, actions, children, }: { title: string; defaultOpen?: boolean; keepMounted?: boolean; + /** Reopens the section each time a link reveals it. */ + revealCount?: number; actions?: ReactNode; children: ReactNode; }) { - const [open, setOpen] = useState(defaultOpen); + // A reveal opens at once: the opening animation would move what was just scrolled to. + const [open, setOpen] = useState( + revealCount > 0 ? "revealed" : defaultOpen, + ); + const [seenRevealCount, setSeenRevealCount] = useState(revealCount); + if (revealCount !== seenRevealCount) { + setSeenRevealCount(revealCount); + setOpen("revealed"); + } const headingRef = useRef(null); const setOpenWithScrollAnchor = (nextOpen: boolean) => { if (!nextOpen) { @@ -387,7 +400,7 @@ function Section({ }; return ( } data-pull-request-summary-section @@ -408,7 +421,7 @@ function Section({ {actions}
- +
{children}
 @@ -422,6 +435,8 @@ const CommentFilterSchema = Schema.Struct({ bots: Schema.Boolean, resolved: Sche const DEFAULT_COMMENT_FILTER: typeof CommentFilterSchema.Type = { bots: false, resolved: false }; export function PullRequestSummaryTab({ + revealUrl, + onRevealSettled, scrollerRef, environmentId, threadRef, @@ -438,6 +453,9 @@ export function PullRequestSummaryTab({ onRefresh, onRefreshChecks = onRefresh, }: { + /** A clicked link to scroll to, until onRevealSettled reports it shown. */ + revealUrl?: string | undefined; + onRevealSettled?: ((revealUrl: string) => void) | undefined; scrollerRef: (node: HTMLDivElement | null) => void; environmentId: EnvironmentId; threadRef: ScopedThreadRef | null; @@ -562,6 +580,60 @@ export function PullRequestSummaryTab({ const [commentSaving, setCommentSaving] = useState(false); const editingCommentId = commentScope?.pullRequest === detail.url ? commentScope.commentId : null; + const scrollRef = useRef(null); + const location = parsePullRequestAnchor(revealUrl); + const [checksReveals, setChecksReveals] = useState(0); + const [descriptionReveals, setDescriptionReveals] = useState(0); + const [seenRevealUrl, setSeenRevealUrl] = useState(undefined); + if (revealUrl !== seenRevealUrl) { + setSeenRevealUrl(revealUrl); + if (location?.kind === "checks") setChecksReveals((count) => count + 1); + if (location?.kind === "summary" && location.anchor !== "top") { + setDescriptionReveals((count) => count + 1); + } + } + const stopHolding = useRef<(() => void) | null>(null); + const revealSummary = useEffectEvent(() => { + const root = scrollRef.current; + if (revealUrl === undefined || !root) return; + let target: HTMLElement | null | undefined; + if (location?.kind === "checks") { + target = root.querySelector( + '[data-pull-request-summary-section][aria-label="Checks"]', + ); + } else if (location?.kind === "summary") { + if (location.anchor === "top") { + stopHolding.current?.(); + root.scrollTop = 0; + return onRevealSettled?.(revealUrl); + } + target = /^issue-\d+$/.test(location.anchor) + ? root.querySelector("[data-pr-description]") + : [...root.querySelectorAll("[id]")].find( + (node) => node.id === location.anchor || node.id === `user-content-${location.anchor}`, + ); + } + // A heading may still be rendering; the link stays pending until it appears. + if (!target) return; + stopHolding.current?.(); + const heading = target.hasAttribute("data-pull-request-summary-section") + ? null + : target.closest("[data-pull-request-summary-section]")?.firstElementChild; + stopHolding.current = holdLinkedNodeInView( + target, + root, + () => (heading?.getBoundingClientRect().height ?? 0) + (heading ? 8 : 0), + ); + onRevealSettled?.(revealUrl); + }); + const editingDescription = bodyScope?.url === detail.url; + // oxlint-disable-next-line t3code/no-unreasoned-state-sync -- scrolls to a linked section, description or heading once it renders + useEffect(() => { + revealSummary(); + // oxlint-disable-next-line react/exhaustive-effect-dependencies -- retries once the description markdown renders, including after its editor closes + }, [revealUrl, description, checksReveals, descriptionReveals, editingDescription]); + // oxlint-disable-next-line t3code/no-unreasoned-state-sync -- the hold observes DOM outside React and must end with the tab + useEffect(() => () => stopHolding.current?.(), []); // Checkbox ticks and the editor share one draft, so each starts from the other's latest body. const saveDescription = async (body: string) => { const key = detail.url; @@ -733,7 +805,14 @@ export function PullRequestSummaryTab({ }; return ( -
+
{ + scrollRef.current = node; + scrollerRef(node); + }} + className="h-full overflow-y-auto" + data-pull-request-summary-scroll + >
} label="Reviewers"> @@ -838,9 +917,14 @@ export function PullRequestSummaryTab({
-
+
- {bodyScope?.url === detail.url ? ( + {editingDescription ? ( setBodyScope(null)} /> ) : ( -
+
0 ? description : "_No description provided._"} @@ -875,7 +959,12 @@ export function PullRequestSummaryTab({
-
+
{checksStale ? (
Check details are out of date. diff --git a/apps/web/src/components/pullRequest/PullRequestTimelineTab.browser.test.tsx b/apps/web/src/components/pullRequest/PullRequestTimelineTab.browser.test.tsx index 4e8f1e4319c5c..847cbb60aa650 100644 --- a/apps/web/src/components/pullRequest/PullRequestTimelineTab.browser.test.tsx +++ b/apps/web/src/components/pullRequest/PullRequestTimelineTab.browser.test.tsx @@ -4,7 +4,7 @@ import { SourceControlProviderKind, type PullRequestDetailView, } from "@t3tools/contracts"; -import { expect, it } from "vite-plus/test"; +import { expect, it, vi } from "vite-plus/test"; import { render } from "../../../test/render"; import "../../index.css"; @@ -87,3 +87,90 @@ it("mounts only the timeline rows near the viewport of a huge pull request", asy expect(mounted).toBeGreaterThan(0); expect(mounted).toBeLessThan(COMMITS / 10); }); + +it.each(["review", "issue-comment", "resolved-reply"] as const)( + "reveals a linked %s below the virtual viewport", + async (kind) => { + const url = `${detail.url}#${kind === "review" ? "pullrequestreview-123" : kind === "issue-comment" ? "issuecomment-123" : "discussion_r123"}`; + const linkedDetail: PullRequestDetailView = { + ...detail, + comments: + kind === "resolved-reply" + ? [] + : [ + { + id: "linked", + kind, + author: detail.author, + body: "Linked reply", + createdAt: "2026-10-01T00:00:00Z", + url, + path: null, + reviewState: kind === "review" ? "CHANGES_REQUESTED" : null, + }, + ], + reviewThreads: + kind !== "resolved-reply" + ? [] + : [ + { + id: "thread", + path: "src/test.ts", + line: 5, + side: "right", + isResolved: true, + isOutdated: false, + comments: [ + { + id: "root", + author: detail.author, + body: "Root comment", + createdAt: "2026-10-01T00:00:00Z", + url: `${detail.url}#discussion_r122`, + }, + { + id: "reply", + author: detail.author, + body: "Linked reply", + createdAt: "2026-10-01T00:01:00Z", + url, + }, + ], + }, + ], + }; + const settled = vi.fn(); + await using screen = await render( +
+ {}} + detail={linkedDetail} + environmentId={EnvironmentId.make("environment")} + reference={detail} + order="oldest" + onOpenCommit={() => {}} + onRefresh={() => {}} + revealUrl={url} + onRevealSettled={settled} + /> +
, + ); + await expect.element(screen.getByText("Linked reply", { exact: true })).toBeVisible(); + await vi.waitFor(() => expect(settled).toHaveBeenCalledWith(url)); + await screen.rerender( +
+ {}} + detail={linkedDetail} + environmentId={EnvironmentId.make("environment")} + reference={detail} + order="oldest" + onOpenCommit={() => {}} + onRefresh={() => {}} + onRevealSettled={settled} + /> +
, + ); + await expect.element(screen.getByText("Linked reply", { exact: true })).toBeVisible(); + }, +); diff --git a/apps/web/src/components/pullRequest/PullRequestTimelineTab.tsx b/apps/web/src/components/pullRequest/PullRequestTimelineTab.tsx index 36dca98e23179..df0e9c260d88b 100644 --- a/apps/web/src/components/pullRequest/PullRequestTimelineTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestTimelineTab.tsx @@ -1,3 +1,9 @@ +import { + parsePullRequestAnchor, + pullRequestAnchorMatchesUrl, + resolvePullRequestReviewAnchor, +} from "@t3tools/shared/pullRequestAnchor"; +import { holdLinkedNodeInView } from "./holdLinkedNodeInView"; import { capitalizeTerm, sourceControlClients, @@ -20,7 +26,7 @@ import { MessageSquareIcon, MessageSquarePlusIcon, } from "lucide-react"; -import { useState, type ReactNode } from "react"; +import { useEffect, useEffectEvent, useRef, useState, type ReactNode } from "react"; import { cn } from "~/lib/utils"; import { readLocalApi } from "~/localApi"; @@ -35,6 +41,7 @@ import { Tooltip, TooltipPopup, TooltipTrigger } from "../ui/tooltip"; import { buildPullRequestTimeline, groupPullRequestTimelineConversations, + pullRequestTimelineEventMatches, type PullRequestTimelineRow, isPullRequestVerdictStale, newestPullRequestCommitAt, @@ -66,6 +73,7 @@ import { import { LegendList, type LegendListRef } from "@legendapp/list/react"; interface ReactionSurface { + revealAnchor?: string | null; readonly canReact: boolean; /** Absent where there is no composer beside the panel to add a remark to. */ readonly onAddToChat?: ((comment: PullRequestComment) => void) | undefined; @@ -259,7 +267,16 @@ function ConversationCard({ }; return ( -
+
@@ -339,19 +356,25 @@ function uniqueConversationActors(events: ReadonlyArray; editable: ReadonlyMap; cwd: string; onOpen: (url: string) => void; reactions: ReactionSurface; }) { - const [open, setOpen] = useState(false); + const [open, setOpen] = useState( + () => + revealAnchor != null && + events.some((event) => pullRequestAnchorMatchesUrl(revealAnchor, event.url)), + ); const actors = uniqueConversationActors(events); const first = events[0]; if (first === undefined) return null; @@ -413,6 +436,7 @@ function ConversationGroup({ } function ReviewThreadEvent({ + revealAnchor, event, thread, editable, @@ -420,6 +444,7 @@ function ReviewThreadEvent({ onOpen, reactions, }: { + revealAnchor?: string | null; event: PullRequestTimelineEvent; thread: PullRequestTimelineThread; editable: ReadonlyMap; @@ -428,7 +453,14 @@ function ReviewThreadEvent({ reactions: ReactionSurface; }) { const dismissed = pullRequestReviewOutcome(event.reviewState) === "dismissed"; - const [expanded, setExpanded] = useState(!thread.isResolved && !dismissed); + const [expanded, setExpanded] = useState( + () => + (revealAnchor != null && + [event, ...thread.replies].some((item) => + pullRequestAnchorMatchesUrl(revealAnchor, item.url), + )) || + (!thread.isResolved && !dismissed), + ); const replyCount = thread.replies.length + thread.moreReplies; return ( @@ -675,6 +707,8 @@ function ReviewVerdictEvent({ } export function PullRequestTimelineTab({ + revealUrl, + onRevealSettled, scrollerRef, detail, environmentId, @@ -685,6 +719,9 @@ export function PullRequestTimelineTab({ onAddToChat, onRefresh, }: { + /** A clicked link to scroll to and mark, until onRevealSettled reports it shown. */ + revealUrl?: string | undefined; + onRevealSettled?: ((revealUrl: string) => void) | undefined; scrollerRef: (node: HTMLElement | null) => void; detail: PullRequestDetailView; environmentId: EnvironmentId; @@ -696,9 +733,29 @@ export function PullRequestTimelineTab({ onRefresh: () => void; }) { const events = buildPullRequestTimeline(detail); + const location = parsePullRequestAnchor(revealUrl); + const revealAnchor = location?.kind === "timeline" ? location.anchor : null; + // The linked item stays marked and open after it is shown, until another link replaces it. + const [linked, setLinked] = useState<{ readonly anchor: string; readonly count: number } | null>( + () => (revealAnchor === null ? null : { anchor: revealAnchor, count: 1 }), + ); + const [seenRevealUrl, setSeenRevealUrl] = useState(revealUrl); + if (revealUrl !== seenRevealUrl) { + setSeenRevealUrl(revealUrl); + // Each click remounts the linked row, so it opens again even after the user closed it. + if (revealAnchor !== null) setLinked({ anchor: revealAnchor, count: (linked?.count ?? 0) + 1 }); + } + const anchor = linked + ? resolvePullRequestReviewAnchor(linked.anchor, detail.comments, detail.reviewThreads) + : null; + const listRef = useRef(null); + const matches = (event: PullRequestTimelineEvent) => + anchor !== null && pullRequestTimelineEventMatches(event, anchor); + const term = sourceControlClients.get(detail.provider).changeRequest.singular; const newestCommitAt = newestPullRequestCommitAt(detail.commits); const reactions: ReactionSurface = { + revealAnchor: anchor, canReact: detail.capabilities.reactions === true, onAddToChat: onAddToChat && ((comment) => onAddToChat({ kind: "comment", comment })), environmentId, @@ -716,23 +773,48 @@ export function PullRequestTimelineTab({ ); const orderedEvents = order === "newest" ? events : events.toReversed(); const rows = groupPullRequestTimelineConversations(orderedEvents); + const targetIndex = rows.findIndex((row) => + row.kind === "comments" ? row.events.some(matches) : matches(row.event), + ); + const stopHolding = useRef<(() => void) | null>(null); + const reveal = useEffectEvent(() => { + const list = listRef.current; + const url = revealUrl; + if (url === undefined || revealAnchor === null || targetIndex < 0 || !list) return; + stopHolding.current?.(); + void list.scrollToIndex({ index: targetIndex, viewPosition: 0, animated: false }).then(() => { + const scroller = list.getScrollableNode(); + const node = scroller?.querySelector("[data-pr-link-target=true]"); + if (scroller && node) stopHolding.current = holdLinkedNodeInView(node, scroller, () => 16); + onRevealSettled?.(url); + }); + }); + // oxlint-disable-next-line t3code/no-unreasoned-state-sync -- reveals a link destination after activity loading and virtual list mounting + useEffect(() => { + reveal(); + // oxlint-disable-next-line react/exhaustive-effect-dependencies -- runs for each clicked link and again when activity resolves its row + }, [revealUrl, targetIndex]); + // oxlint-disable-next-line t3code/no-unreasoned-state-sync -- the hold observes DOM outside React and must end with the tab + useEffect(() => () => stopHolding.current?.(), []); const openOnHost = (url: string) => { void readLocalApi()?.shell.openExternal(url); }; const rowKey = (row: PullRequestTimelineRow) => { - if (row.kind === "comments") return `comments:${row.events[0]?.id ?? "empty"}`; + if (row.kind === "comments") + return `comments:${row.events[0]?.id ?? "empty"}:${row.events.some(matches) ? (linked?.count ?? "") : ""}`; const event = row.event; // A thread's resolution or dismissal remounts it, so its disclosure starts in the new state. return event.thread === undefined - ? event.id - : `thread:${event.id}:${event.thread.isResolved}:${pullRequestReviewOutcome(event.reviewState) === "dismissed"}`; + ? `${event.id}:${matches(event) ? (linked?.count ?? "") : ""}` + : `thread:${event.id}:${matches(event) ? (linked?.count ?? "") : ""}:${event.thread.isResolved}:${pullRequestReviewOutcome(event.reviewState) === "dismissed"}`; }; const renderRow = ({ item: row }: { item: PullRequestTimelineRow }) => { if (row.kind === "comments") { return ( - ref={(list: LegendListRef | null) => scrollerRef(list?.getScrollableNode() ?? null)} + ref={(list: LegendListRef | null) => { + listRef.current = list; + scrollerRef(list?.getScrollableNode() ?? null); + }} data={rows} extraData={{ editable, reactions, newestCommitAt }} keyExtractor={rowKey} @@ -793,7 +879,15 @@ export function PullRequestTimelineTab({ estimatedItemSize={140} className="scrollbar-gutter-both h-full px-4" contentContainerClassName="relative mx-auto w-full max-w-3xl before:absolute before:bottom-10 before:left-[15px] before:top-6 before:w-px before:bg-border/45" - ListHeaderComponent={
} + ListHeaderComponent={ + anchor !== null && targetIndex < 0 && revealUrl === undefined ? ( +

+ The linked item is not in the loaded activity. +

+ ) : ( +
+ ) + } ListFooterComponent={
} /> ); diff --git a/apps/web/src/components/pullRequest/PullRequestsPage.tsx b/apps/web/src/components/pullRequest/PullRequestsPage.tsx index 2af8383bebcd4..0d494d923e33f 100644 --- a/apps/web/src/components/pullRequest/PullRequestsPage.tsx +++ b/apps/web/src/components/pullRequest/PullRequestsPage.tsx @@ -1027,10 +1027,13 @@ export function PullRequestsPage() { if (rightPanelState.isOpen) setMaximized((value) => !value); }; const onGuideShown = () => setMaximized(true); + // updateSearch leaves selectedUrl out, so a reload does not reveal the link again. + const consumeLinkedUrl = useEffectEvent(() => updateSearch({})); // oxlint-disable-next-line t3code/no-unreasoned-state-sync -- opens the URL-linked pull request in the right panel store useEffect(() => { if (!pullRequestsSupported || rightPanelRef === null || linkedSelection === null) return; useRightPanelStore.getState().openPullRequest(rightPanelRef, linkedSelection); + if (linkedSelection.revealUrl !== undefined) consumeLinkedUrl(); }, [linkedSelection, pullRequestsSupported, rightPanelRef]); const selected = @@ -1521,6 +1524,13 @@ export function PullRequestsPage() { getShortcutContext={getShortcutContext} shortcutsEnabled={activePullRequestSurface?.id === renderedPullRequestSurface.id} key={renderedPullRequestSurface.id} + revealUrl={renderedPullRequestSurface.revealUrl} + onRevealSettled={(revealUrl) => { + if (rightPanelRef === null) return; + useRightPanelStore + .getState() + .settlePullRequestReveal(rightPanelRef, renderedPullRequestSurface.id, revealUrl); + }} environmentId={panelEnvironmentId} onGuideShown={onGuideShown} onSelectPullRequest={(reference) => { diff --git a/apps/web/src/components/pullRequest/holdLinkedNodeInView.ts b/apps/web/src/components/pullRequest/holdLinkedNodeInView.ts new file mode 100644 index 0000000000000..4261cb2fb0da8 --- /dev/null +++ b/apps/web/src/components/pullRequest/holdLinkedNodeInView.ts @@ -0,0 +1,39 @@ +import { observeResize } from "~/lib/observeResize"; + +const USER_SCROLL_INTENT = ["wheel", "touchstart", "pointerdown", "keydown"] as const; + +/** + * Keeps a linked node `offset()` below the scroller's top while the content around it opens and + * its markdown loads, until the user scrolls, the node leaves, or the returned stop runs. Only the + * scroller moves; scrollIntoView would also scroll the panel's clipped ancestors. + */ +export function holdLinkedNodeInView( + node: HTMLElement, + scroller: HTMLElement, + offset: () => number, +): () => void { + const controller = new AbortController(); + const stop = () => { + unobserve(); + controller.abort(); + }; + const align = () => { + if (!node.isConnected) return stop(); + scroller.scrollTop += + node.getBoundingClientRect().top - scroller.getBoundingClientRect().top - offset(); + }; + const unobserve = observeResize([...scroller.children], align); + for (const type of USER_SCROLL_INTENT) { + scroller.addEventListener(type, stop, { passive: true, signal: controller.signal }); + } + align(); + // Activity can load after the click; by then the user may be typing elsewhere. + const active = document.activeElement; + if ( + !(active instanceof HTMLElement) || + !(active.isContentEditable || active.matches("input, textarea, select")) + ) { + node.focus({ preventScroll: true }); + } + return stop; +} diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts index 5b298d23c429b..6ea7d6a5b0e1a 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts @@ -50,6 +50,7 @@ import { shouldRefreshPullRequestActivity, resolveBaseFreshness, buildPullRequestTimeline, + pullRequestLinkSubject, writePullRequestDetailSnapshot, readableFailure, } from "./pullRequestDetail.logic"; @@ -313,6 +314,58 @@ describe("review verdicts", () => { }); describe("pull request timeline", () => { + it("names the comment or commit a link points at for its hover card", () => { + const pr = "https://github.com/o/r/pull/1"; + const sha = "a".repeat(40); + const activity = { + comments: [ + { + ...TIMELINE_SOURCE.comments[0]!, + url: `${pr}#issuecomment-5`, + body: "\n> [!NOTE]\n> **Looks** good, see `run()` ![shot](x.png)", + }, + ], + reviewThreads: [ + { + id: "t", + path: "a.ts", + line: 1, + side: "right" as const, + isResolved: false, + isOutdated: false, + comments: [ + { + id: "r1", + author: { login: "reviewer", name: null, avatarUrl: null }, + body: "nit", + createdAt: "2026-07-03T00:00:00Z", + url: `${pr}#discussion_r1`, + reviewUrl: `${pr}#pullrequestreview-9`, + }, + ], + }, + ], + commits: [{ oid: sha, messageHeadline: "fix it", committedDate: "2026-07-02T00:00:00Z" }], + }; + expect(pullRequestLinkSubject(`${pr}#issuecomment-5`, activity)).toMatchObject({ + kind: "comment", + excerpt: "Looks good, see run()", + }); + // A review with no text of its own is shown through the comment it posted. + expect(pullRequestLinkSubject(`${pr}#pullrequestreview-9`, activity)).toMatchObject({ + kind: "comment", + author: { login: "reviewer" }, + excerpt: "nit", + }); + expect(pullRequestLinkSubject(`${pr}/commits/${sha}`, activity)).toMatchObject({ + kind: "commit", + commit: { messageHeadline: "fix it" }, + }); + for (const url of [pr, `${pr}/files`, `${pr}/checks`, `${pr}#issuecomment-6`]) { + expect(pullRequestLinkSubject(url, activity), url).toBeNull(); + } + }); + it("orders host timestamps by instant across offsets and fractional seconds", () => { const events = buildPullRequestTimeline({ ...TIMELINE_SOURCE, diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts index 537de10a2a8c1..cceadfbee3c60 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts @@ -1,3 +1,9 @@ +import { + parsePullRequestAnchor, + pullRequestAnchorMatchesUrl, + pullRequestLinkCommit, + resolvePullRequestReviewAnchor, +} from "@t3tools/shared/pullRequestAnchor"; import { stripInlineContextReferences } from "~/lib/composerContextReferences"; import type { Nodes as MdastNodes } from "mdast"; import { fromMarkdown } from "mdast-util-from-markdown"; @@ -17,6 +23,7 @@ import { type PullRequestCheck, type PullRequestChecksState as ContractPullRequestChecksState, type PullRequestComment, + type PullRequestActivity, type PullRequestCommit, type PullRequestContextMetadata, type PullRequestDetailView, @@ -694,6 +701,80 @@ function readableBody(raw: string): string | null { * Merged wins over closed: GitHub sets both timestamps on a merge, and reporting "closed" for a * merged pull request would misstate what happened. */ +export type PullRequestLinkSubject = + | { + readonly kind: "comment"; + readonly author: PullRequestActor | null; + readonly createdAt: string; + readonly reviewState: string | null; + readonly excerpt: string | null; + } + | { readonly kind: "commit"; readonly commit: PullRequestCommit }; + +const LINK_EXCERPT_LENGTH = 280; + +/** The words of a markdown body, without markup, HTML or images, for a short preview. */ +function markdownExcerpt(body: string): string | null { + const words: string[] = []; + const visit = (node: MdastNodes) => { + if (node.type === "text" || node.type === "inlineCode") words.push(node.value); + if (node.type !== "code" && "children" in node) for (const child of node.children) visit(child); + }; + visit(fromMarkdown(body)); + // GitHub's alert marker (`> [!NOTE]`) parses as text but reads as markup. + const text = words + .join(" ") + .replace(/\[!(?:note|tip|important|warning|caution)\]/giu, "") + .replace(/\s+/gu, " ") + .trim(); + if (text === "") return null; + return text.length > LINK_EXCERPT_LENGTH ? `${text.slice(0, LINK_EXCERPT_LENGTH - 1)}…` : text; +} + +/** The comment or commit a pull request link names, for its hover card; null for the PR itself. */ +export function pullRequestLinkSubject( + url: string, + activity: Pick, +): PullRequestLinkSubject | null { + const oid = pullRequestLinkCommit(url)?.toLowerCase(); + if (oid !== undefined) { + const commit = activity.commits.find((candidate) => candidate.oid.toLowerCase() === oid); + return commit === undefined ? null : { kind: "commit", commit }; + } + const location = parsePullRequestAnchor(url); + if (location?.kind !== "timeline") return null; + const anchor = resolvePullRequestReviewAnchor( + location.anchor, + activity.comments, + activity.reviewThreads, + ); + const comment = activity.comments.find((candidate) => + pullRequestAnchorMatchesUrl(anchor, candidate.url), + ); + const threadComment = activity.reviewThreads + .flatMap((thread) => thread.comments) + .find((candidate) => pullRequestAnchorMatchesUrl(anchor, candidate.url)); + const found = comment ?? threadComment; + if (found === undefined) return null; + return { + kind: "comment", + author: found.author, + createdAt: found.createdAt, + reviewState: comment?.reviewState ?? null, + excerpt: markdownExcerpt(found.body), + }; +} + +/** Whether a link anchor names this event or a reply in its review thread. */ +export function pullRequestTimelineEventMatches( + event: PullRequestTimelineEvent, + anchor: string, +): boolean { + return [event, ...(event.thread?.replies ?? [])].some((item) => + pullRequestAnchorMatchesUrl(anchor, item.url), + ); +} + export function buildPullRequestTimeline( detail: Pick< PullRequestDetailView, diff --git a/apps/web/src/components/pullRequest/usePullRequestListView.ts b/apps/web/src/components/pullRequest/usePullRequestListView.ts index fe22fe5af2838..2fad034b2362e 100644 --- a/apps/web/src/components/pullRequest/usePullRequestListView.ts +++ b/apps/web/src/components/pullRequest/usePullRequestListView.ts @@ -40,6 +40,7 @@ import { pullRequestFilterProjects } from "@t3tools/client-runtime/pull-request- import { pullRequestListEntryId } from "./pullRequestListGroups.logic"; export interface PullRequestsSearch extends PullRequestListPreferences { + readonly selectedUrl?: string; readonly environmentId?: EnvironmentId; readonly projectId?: ProjectId; readonly host?: string; @@ -139,6 +140,16 @@ export function usePullRequestScope(search: PullRequestsSearch) { const assignmentKey = environmentQueries .map(({ environmentId, projectIds }) => `${environmentId}#${projectIds?.join("+") ?? "*"}`) .join("|"); + // The URL is user-controlled and stands in for the PR link until detail loads. + const selectedLink = search.selectedUrl + ? sourceControlClients.parseChangeRequestUrl(search.selectedUrl)?.link + : undefined; + const selectedUrl = + selectedLink !== undefined && + selectedLink.number === search.number && + selectedLink.repository.toLowerCase() === search.repository?.toLowerCase() + ? search.selectedUrl + : undefined; const linkedSelection = search.repository && search.number && selectedProject ? { @@ -146,6 +157,7 @@ export function usePullRequestScope(search: PullRequestsSearch) { repository: search.repository, number: search.number, projectId: selectedProject.id, + ...(selectedUrl ? { url: selectedUrl, revealUrl: selectedUrl } : {}), ...(selectedHost ? { host: selectedHost } : {}), } : null; diff --git a/apps/web/src/lib/openPullRequestLink.browser.test.tsx b/apps/web/src/lib/openPullRequestLink.browser.test.tsx index a93f32a8902f7..7d93882b12b15 100644 --- a/apps/web/src/lib/openPullRequestLink.browser.test.tsx +++ b/apps/web/src/lib/openPullRequestLink.browser.test.tsx @@ -85,6 +85,8 @@ it.each([ ["shiftKey", commitUrl], ["altKey", commitUrl], ["shiftKey", pullRequestCommitUrl], + ["shiftKey", `${pullRequestCommitUrl}#diff-abcR5`], + ["altKey", `${pullRequestCommitUrl}#diff-abcR5`], ] as const)("keeps %s clicks on %s native", async (modifier, url) => { setup(); await using screen = await render( diff --git a/apps/web/src/lib/openPullRequestLink.ts b/apps/web/src/lib/openPullRequestLink.ts index 31c04b6263e5f..74a0b29d7aa22 100644 --- a/apps/web/src/lib/openPullRequestLink.ts +++ b/apps/web/src/lib/openPullRequestLink.ts @@ -1,3 +1,4 @@ +import { parsePullRequestAnchor } from "@t3tools/shared/pullRequestAnchor"; import type { EnvironmentId, ScopedThreadRef } from "@t3tools/contracts"; import { useAtomValue } from "@effect/atom-react"; import { useNavigate } from "@tanstack/react-router"; @@ -89,6 +90,7 @@ export function useOpenChangeRequestLink( }); if (target === null) return false; const { project, repository, number, host } = target; + const revealUrl = parsePullRequestAnchor(targetUrl) === null ? undefined : targetUrl; const pullRequestsSearch = { repository, number, @@ -106,6 +108,7 @@ export function useOpenChangeRequestLink( ...(host === undefined ? {} : { host }), repository, url: targetUrl, + ...(revealUrl === undefined ? {} : { revealUrl }), number, }); if (!resolvedThreadRef) { @@ -124,7 +127,12 @@ export function useOpenChangeRequestLink( } void navigate({ to: "/pull-requests", - search: { involvement: "all", state: "all", ...pullRequestsSearch }, + search: { + involvement: "all", + state: "all", + ...pullRequestsSearch, + ...(revealUrl === undefined ? {} : { selectedUrl: revealUrl }), + }, }); return true; }; @@ -140,6 +148,14 @@ export function useOpenChangeRequestLink( const parsedCommit = sourceControlClients.parseCommitUrl(targetUrl, identity?.provider); // Shift and Alt keep their browser meaning (new window, download) on commit links. if (parsedCommit !== null && (event.shiftKey || event.altKey)) return false; + // A plain commit link stays a workspace commit; one naming a file or line opens the PR's diff. + const anchor = parsePullRequestAnchor(targetUrl); + if (anchor !== null && anchor.kind !== "code" && openPullRequest()) { + event.preventDefault(); + event.stopPropagation(); + return true; + } + if ( parsedCommit !== null && commitThread && diff --git a/apps/web/src/rightPanelStore.test.ts b/apps/web/src/rightPanelStore.test.ts index ec0f4e87c12e8..3a6bd304863b0 100644 --- a/apps/web/src/rightPanelStore.test.ts +++ b/apps/web/src/rightPanelStore.test.ts @@ -1009,6 +1009,45 @@ describe("rightPanelStore", () => { selectActiveRightPanelSurface(useRightPanelStore.getState().byThreadKey, refA), ).toMatchObject({ url }); expect(state.surfaces[1]).not.toHaveProperty("url"); + expect(state.surfaces[0]).not.toHaveProperty("revealUrl"); + }); + + it("holds a clicked link until the panel settles it, and a later link replaces it", async () => { + const pr = { projectId: "project-a", repository: "pingdotgg/t3code", number: 4909 }; + const id = pullRequestSurfaceId(pr); + const url = "https://github.com/pingdotgg/t3code/pull/4909"; + const surface = () => + selectActiveRightPanelSurface(useRightPanelStore.getState().byThreadKey, refA); + const comment = (n: number) => `${url}#issuecomment-${n}`; + const store = useRightPanelStore.getState(); + store.openPullRequest(refA, { ...pr, url: comment(1), revealUrl: comment(1) }); + expect(surface()).toMatchObject({ url: comment(1), revealUrl: comment(1) }); + store.openPullRequest(refA, { ...pr, url }); + expect(surface()).not.toHaveProperty("revealUrl"); + store.openPullRequest(refA, { ...pr, url: comment(2), revealUrl: comment(2) }); + // A host read started for an earlier link settles only that link. + store.settlePullRequestReveal(refA, id, comment(1)); + expect(surface()).toMatchObject({ revealUrl: comment(2) }); + store.settlePullRequestReveal(refA, id, comment(2)); + expect(surface()).toMatchObject({ url: comment(2) }); + expect(surface()).not.toHaveProperty("revealUrl"); + // Reopening a closed tab hands back its surface, whose url still names the comment. + store.openPullRequest(refA, { ...pr, url: comment(2) }); + expect(surface()).not.toHaveProperty("revealUrl"); + store.openPullRequest(refA, { ...pr, url: comment(3), revealUrl: comment(3) }); + await reloadRightPanelStore(); + expect(surface()).toMatchObject({ url: comment(3) }); + expect(surface()).not.toHaveProperty("revealUrl"); + store.openPullRequest(refA, { ...pr, url: comment(4), revealUrl: comment(4) }); + store.closeSurface(refA, id); + await reopenClosedView(useClosedViewStore.getState().entries[0]!, { + workspaceAvailable: true, + openPreview: () => { + throw new Error("A PR tab must not create a browser session"); + }, + }); + expect(surface()).toMatchObject({ url: comment(4) }); + expect(surface()).not.toHaveProperty("revealUrl"); }); it("restores each environment, thread and PR surface's detail tab after reload and URL reopen", async () => { diff --git a/apps/web/src/rightPanelStore.ts b/apps/web/src/rightPanelStore.ts index 1f736d4f119ae..7de186174d348 100644 --- a/apps/web/src/rightPanelStore.ts +++ b/apps/web/src/rightPanelStore.ts @@ -73,6 +73,8 @@ export type RightPanelSurface = repository: string; number: number; url?: string; + /** A clicked link waiting for the panel to show where it points; cleared once shown. */ + revealUrl?: string; detailTab?: PullRequestDetailTab; } | { id: "pull-requests"; kind: "pull-requests" } @@ -148,6 +150,8 @@ interface RightPanelStoreState { repository: string; number: number; url?: string; + /** A clicked link to reveal; reopening a surface passes only its `url`. */ + revealUrl?: string; detailTab?: PullRequestDetailTab; }, ) => void; @@ -156,6 +160,8 @@ interface RightPanelStoreState { surfaceId: string, detailTab: PullRequestDetailTab, ) => void; + /** Clears the surface's pending link only while it is still `revealUrl`. */ + settlePullRequestReveal: (ref: ScopedThreadRef, surfaceId: string, revealUrl: string) => void; openTerminal: (ref: ScopedThreadRef, terminalId: string) => void; splitTerminal: ( ref: ScopedThreadRef, @@ -293,6 +299,13 @@ export function pullRequestSurface(target: { }; } +/** A pending link belongs to the click that made it, not to a reload or a reopen. */ +const withoutReveal = (surface: RightPanelSurface): RightPanelSurface => { + if (surface.kind !== "pull-request" || surface.revealUrl === undefined) return surface; + const { revealUrl: _pending, ...rest } = surface; + return rest; +}; + const upsertSurface = ( current: ThreadRightPanelState, surface: RightPanelSurface, @@ -752,16 +765,17 @@ export const useRightPanelStore = create()( userAction(state, scopedThreadKey(ref), (current) => { const surface = pullRequestSurface(target); const next = upsertSurface(current, surface); - return target.url - ? { - ...next, - surfaces: next.surfaces.map((entry) => - entry.id === surface.id && entry.kind === "pull-request" - ? { ...entry, ...surface } - : entry, - ), - } - : next; + const { url, revealUrl } = target; + if (!url && !revealUrl) return next; + return { + ...next, + surfaces: next.surfaces.map((entry) => { + if (entry.id !== surface.id || entry.kind !== "pull-request") return entry; + // A later link replaces one not yet shown. + const { revealUrl: _previous, ...rest } = entry; + return { ...rest, ...surface, ...(revealUrl ? { revealUrl } : {}) }; + }), + }; }), ), setPullRequestDetailTab: (ref, surfaceId, detailTab) => @@ -781,6 +795,20 @@ export const useRightPanelStore = create()( ), })); }), + settlePullRequestReveal: (ref, surfaceId, revealUrl) => + set((state) => { + const threadKey = scopedThreadKey(ref); + const existing = state.byThreadKey[threadKey]?.surfaces.find( + (surface) => surface.id === surfaceId, + ); + if (existing?.kind !== "pull-request" || existing.revealUrl !== revealUrl) return state; + return automaticUpdate(state, threadKey, (current) => ({ + ...current, + surfaces: current.surfaces.map((surface) => + surface.id === surfaceId ? withoutReveal(surface) : surface, + ), + })); + }), openFile: (ref, requestedPath, line) => set((state) => userAction(state, scopedThreadKey(ref), (current) => { @@ -1164,8 +1192,15 @@ export const useRightPanelStore = create()( ), partialize: (state) => ({ byThreadKey: Object.fromEntries( - Object.entries(state.byThreadKey).filter( - ([threadKey]) => !isPullRequestsPanelKey(threadKey), + Object.entries(state.byThreadKey).flatMap(([threadKey, threadState]) => + isPullRequestsPanelKey(threadKey) + ? [] + : [ + [ + threadKey, + { ...threadState, surfaces: threadState.surfaces.map(withoutReveal) }, + ], + ], ), ), threadPanelVisibilityByThreadKey: Object.fromEntries( @@ -1195,7 +1230,7 @@ useRightPanelStore.subscribe((next, previous) => { ...removed.filter((surface) => surface.id !== before.activeSurfaceId), ...removed.filter((surface) => surface.id === before.activeSurfaceId), ]; - for (const surface of closedInOrder) { + for (const surface of closedInOrder.map(withoutReveal)) { if ( surface.kind === "terminal" || (surface.kind === "preview" && surface.resourceId !== null) diff --git a/apps/web/src/routes/_chat.pull-requests.tsx b/apps/web/src/routes/_chat.pull-requests.tsx index a95646488ed69..95b72f57992e9 100644 --- a/apps/web/src/routes/_chat.pull-requests.tsx +++ b/apps/web/src/routes/_chat.pull-requests.tsx @@ -29,6 +29,7 @@ function pullRequestSearchLabels(raw: unknown): Partial): PullRequestsSearch => ({ + ...(typeof raw.selectedUrl === "string" ? { selectedUrl: raw.selectedUrl.slice(0, 4096) } : {}), involvement: raw.involvement === "reviewing" || raw.involvement === "authored" ? raw.involvement : "all", state: diff --git a/docs/user/source-control.md b/docs/user/source-control.md index 39731cb3d35b6..e9edbb449dd89 100644 --- a/docs/user/source-control.md +++ b/docs/user/source-control.md @@ -194,6 +194,10 @@ Where you can edit a description, you can also tick its checkboxes from the Summ (the Overview tab on mobile). GitLab calls these merge requests. +A link to part of a pull request opens that part: a comment or review is marked in the activity, a +changed-line link opens the file at those lines, a checks link opens the checks, and a commit link +opens that commit's changes. On web and desktop, hovering a comment or commit link previews it. + On mobile, **Pull requests** lists reviews across your connected environments. Open a review to read its description, activity, commits, and changed files. Line comments stay as drafts until you submit the review. **Work on PR** opens a chat with the review attached; sending diff --git a/packages/contracts/src/pullRequest.ts b/packages/contracts/src/pullRequest.ts index f0f692da3f8da..fd449585180be 100644 --- a/packages/contracts/src/pullRequest.ts +++ b/packages/contracts/src/pullRequest.ts @@ -173,6 +173,8 @@ export const PullRequestThreadComment = Schema.Struct({ createdAt: IsoDateTime, editedAt: Schema.optional(Schema.NullOr(IsoDateTime)), url: Schema.NullOr(Schema.String), + /** The review that posted the comment, so a link to a review with no text finds its comments. */ + reviewUrl: Schema.optional(Schema.NullOr(Schema.String)), reactions: Schema.optional(Schema.Array(PullRequestReaction)), canEdit: Schema.optional(Schema.Boolean), }); diff --git a/packages/shared/package.json b/packages/shared/package.json index 5d9a87ac7719d..f5f769236f906 100644 --- a/packages/shared/package.json +++ b/packages/shared/package.json @@ -3,6 +3,10 @@ "private": true, "type": "module", "exports": { + "./pullRequestAnchor": { + "types": "./src/pullRequestAnchor.ts", + "import": "./src/pullRequestAnchor.ts" + }, "./changedFileKind": { "types": "./src/changedFileKind.ts", "import": "./src/changedFileKind.ts" diff --git a/packages/shared/src/pullRequestAnchor.test.ts b/packages/shared/src/pullRequestAnchor.test.ts new file mode 100644 index 0000000000000..bd81fbc48fd76 --- /dev/null +++ b/packages/shared/src/pullRequestAnchor.test.ts @@ -0,0 +1,135 @@ +import { expect, it } from "vite-plus/test"; +import { + parsePullRequestAnchor, + pullRequestAnchorMatchesUrl, + pullRequestDiffPathMatches, + pullRequestLinkCommit, + resolvePullRequestReviewAnchor, +} from "./pullRequestAnchor.ts"; + +const url = "https://github.com/owner/repo/pull/7"; +it.each(["discussion_r123", "r123", "discussion-diff-123R5-L9", "discussion-diff-123"])( + "resolves review comment alias %s", + (hash) => { + expect(parsePullRequestAnchor(`${url}#${hash}`)).toEqual({ + kind: "timeline", + anchor: "discussion_r123", + }); + expect(pullRequestAnchorMatchesUrl("discussion_r123", `${url}#${hash}`)).toBe(true); + }, +); +it.each([ + "pullrequestreview-123", + "issuecomment-123", + "commitcomment-123", + "event-123", + "note_123", + "comment-123", +])("preserves timeline anchor %s", (anchor) => { + expect(parsePullRequestAnchor(`${url}#${anchor}`)).toEqual({ kind: "timeline", anchor }); +}); +it.each(["a".repeat(32), "b".repeat(64)])( + "reads file and cross-side line ranges for %s", + (digest) => { + expect(parsePullRequestAnchor(`${url}/files#diff-${digest}`)).toEqual({ + kind: "diff", + digest, + range: null, + }); + expect(parsePullRequestAnchor(`${url}/changes#diff-${digest}L12-R15`)).toEqual({ + kind: "diff", + digest, + range: { start: 12, side: "left", end: 15, endSide: "right" }, + }); + expect(parsePullRequestAnchor(`${url}#diff-${digest}R42`)).toEqual({ + kind: "diff", + digest, + range: { start: 42, side: "right", end: 42, endSide: "right" }, + }); + }, +); +it.each([ + [ + "src/hello world.ts", + "bcea8deb9d67b305e28c4a0b0d021615", + "066802cf2cf056706e4e4951cb7dff2717a2ae32fd9797c1f1b41062d57db929", + ], + [ + "src/日本語.ts", + "5c66d5d565666e46924cf80d7bbfdab3", + "191b8ebf2241225db999eaf8b6ff44b07934d577f347d0ff684591eecad34944", + ], + [ + "old/path.ts", + "f5d455478d496859b654884654393a61", + "aa38d67014e2c59d51ea456a155e039f4e32f636a4c450714c3b517bb8bde7e7", + ], +])("matches GitHub path digests for %s", (path, legacy, current) => { + for (const digest of [legacy, current]) { + expect(pullRequestDiffPathMatches(digest, path)).toBe(true); + expect(pullRequestDiffPathMatches(digest, "other.ts")).toBe(false); + } +}); +it("handles encoded hashes and rejects invalid line numbers and URLs", () => { + expect(parsePullRequestAnchor(`${url}#%64iscussion_r123`)).toEqual({ + kind: "timeline", + anchor: "discussion_r123", + }); + expect(parsePullRequestAnchor(`${url}#diff-abcR0`)).toBeNull(); + expect(parsePullRequestAnchor(`${url}#diff-abcR9999999999999999999`)).toBeNull(); + expect(parsePullRequestAnchor("invalid")).toBeNull(); + expect(parsePullRequestAnchor(url)).toBeNull(); + expect(parsePullRequestAnchor(`${url}#unknown`)).toEqual({ kind: "summary", anchor: "unknown" }); + expect(parsePullRequestAnchor(`${url}/files-not-a-tab`)).toBeNull(); + expect(parsePullRequestAnchor(`${url}/commits/main`)).toBeNull(); + expect(parsePullRequestAnchor(`${url}/checks/extra`)).toBeNull(); +}); + +it("keeps the commit scope of a PR diff permalink", () => { + const sha = "a".repeat(40); + expect(pullRequestLinkCommit(`${url}/commits/${sha}#diff-abcR5`)).toBe(sha); + expect(pullRequestLinkCommit(`${url}/files#diff-abcR5`)).toBeNull(); +}); + +const gitlab = "https://gitlab.com/group/repo/-/merge_requests/7"; +it.each([ + [`${url}/files`, "code"], + [`${url}/files/`, "code"], + [`${url}/changes`, "code"], + [`${url}/commits/${"a".repeat(40)}`, "code"], + [`${url}/commits`, "commits"], + [`${url}/checks?check_run_id=1`, "checks"], + [`${gitlab}/diffs`, "code"], + [`${gitlab}/commits`, "commits"], + [`${gitlab}/pipelines`, "checks"], +] as const)("keeps unknown and missing hashes in the tab %s names", (path, kind) => { + for (const hash of ["", "#unknown", "#diff-abcR0", "#diff-abcR9999999999999999999", "#%ZZ"]) { + expect(parsePullRequestAnchor(`${path}${hash}`)).toEqual({ kind }); + } + expect(parsePullRequestAnchor(`${path}#issuecomment-123`)).toEqual({ + kind: "timeline", + anchor: "issuecomment-123", + }); +}); + +it("finds a review with no text of its own through the earliest comment it posted", () => { + const review = `${url}#pullrequestreview-9`; + const threads = [ + { + comments: [ + { url: `${url}#discussion_r3`, createdAt: "2026-07-01T00:02:00Z", reviewUrl: review }, + { url: `${url}#discussion_r2`, createdAt: "2026-07-01T00:01:00Z", reviewUrl: review }, + { url: `${url}#discussion_r1`, createdAt: "2026-07-01T00:00:00Z" }, + ], + }, + ]; + expect(resolvePullRequestReviewAnchor("pullrequestreview-9", [], threads)).toBe("discussion_r2"); + // A review shown in its own right, an unknown review and other anchors are kept. + expect(resolvePullRequestReviewAnchor("pullrequestreview-9", [{ url: review }], threads)).toBe( + "pullrequestreview-9", + ); + expect(resolvePullRequestReviewAnchor("pullrequestreview-8", [], threads)).toBe( + "pullrequestreview-8", + ); + expect(resolvePullRequestReviewAnchor("issuecomment-9", [], threads)).toBe("issuecomment-9"); +}); diff --git a/packages/shared/src/pullRequestAnchor.ts b/packages/shared/src/pullRequestAnchor.ts new file mode 100644 index 0000000000000..37982a7dfd716 --- /dev/null +++ b/packages/shared/src/pullRequestAnchor.ts @@ -0,0 +1,149 @@ +import { md5 } from "@noble/hashes/legacy"; +import { sha256 } from "@noble/hashes/sha2"; +import { bytesToHex, utf8ToBytes } from "@noble/hashes/utils"; + +export interface PullRequestAnchorRange { + readonly start: number; + readonly side: "left" | "right"; + readonly end: number; + readonly endSide: "left" | "right"; +} + +export type PullRequestAnchor = + | { readonly kind: "code" } + | { readonly kind: "commits" } + | { readonly kind: "checks" } + | { + readonly kind: "diff"; + readonly digest: string; + readonly range: PullRequestAnchorRange | null; + } + | { readonly kind: "timeline"; readonly anchor: string } + | { readonly kind: "summary"; readonly anchor: string }; + +function pullRequestUrlHash(url: string | null | undefined): string { + if (!url) return ""; + try { + return decodeURIComponent(new URL(url).hash.slice(1)); + } catch { + return ""; + } +} + +function normalizePullRequestCommentAnchor(anchor: string): string { + return anchor.replace( + /^(?:r|discussion-diff-)(\d+)(?:[LR]\d+(?:-[LR]\d+)?)?$/i, + "discussion_r$1", + ); +} + +// GitHub pull request tabs and their GitLab merge request equivalents. +function pullRequestPathTab(pathname: string): PullRequestAnchor | null { + const tab = /\/(?:pull|-\/merge_requests)\/\d+\/([a-z]+)(?:\/(.*))?$/i.exec(pathname); + const rest = tab?.[2] ?? ""; + switch (tab?.[1]?.toLowerCase()) { + case "files": + case "changes": + case "diffs": + return { kind: "code" }; + case "commits": + if (rest === "") return { kind: "commits" }; + return /^[a-f0-9]{40}\/?$/i.test(rest) ? { kind: "code" } : null; + case "checks": + case "pipelines": + return rest === "" ? { kind: "checks" } : null; + default: + return null; + } +} + +export function parsePullRequestAnchor(url: string | null | undefined): PullRequestAnchor | null { + let parsed: URL; + try { + parsed = new URL(url ?? ""); + } catch { + return null; + } + const fallback = pullRequestPathTab(parsed.pathname); + const anchor = pullRequestUrlHash(url); + if (!anchor) return fallback; + // GitHub line ranges may cross diff sides; file digests were MD5 before SHA-256. + const diff = /^diff-([a-f0-9]+)(?:([LR])(\d+)(?:-([LR])(\d+))?)?$/i.exec(anchor); + if (diff) { + const start = Number(diff[3]); + const end = Number(diff[5] ?? diff[3]); + if ( + diff[2] && + (!Number.isSafeInteger(start) || start < 1 || !Number.isSafeInteger(end) || end < 1) + ) + return fallback; + return { + kind: "diff", + digest: (diff[1] ?? "").toLowerCase(), + range: diff[2] + ? { + start, + side: diff[2].toUpperCase() === "L" ? "left" : "right", + end, + endSide: (diff[4] ?? diff[2]).toUpperCase() === "L" ? "left" : "right", + } + : null, + }; + } + if ( + /^(?:(?:issuecomment-|pullrequestreview-|discussion_r|r|commitcomment-|event-|note_|comment-)\d+|discussion-diff-\d+(?:[LR]\d+(?:-[LR]\d+)?)?)$/i.test( + anchor, + ) + ) { + return { kind: "timeline", anchor: normalizePullRequestCommentAnchor(anchor) }; + } + return fallback ?? { kind: "summary", anchor }; +} + +export function pullRequestAnchorMatchesUrl( + anchor: string, + url: string | null | undefined, +): boolean { + return normalizePullRequestCommentAnchor(pullRequestUrlHash(url)) === anchor; +} + +interface AnchoredComment { + readonly url: string | null; + readonly createdAt: string; + readonly reviewUrl?: string | null | undefined; +} + +/** + * A review with no text of its own appears only through the line comments it posted, so its link + * resolves to the earliest of those. Any other anchor, or a review shown in its own right, is kept. + */ +export function resolvePullRequestReviewAnchor( + anchor: string, + comments: ReadonlyArray>, + reviewThreads: ReadonlyArray<{ readonly comments: ReadonlyArray }>, +): string { + if (!/^pullrequestreview-\d+$/i.test(anchor)) return anchor; + if (comments.some((comment) => pullRequestAnchorMatchesUrl(anchor, comment.url))) return anchor; + // The earliest comment the review posted; `toSorted` is missing from Hermes. + let posted: AnchoredComment | undefined; + for (const comment of reviewThreads.flatMap((thread) => thread.comments)) { + if (comment.url === null || !pullRequestAnchorMatchesUrl(anchor, comment.reviewUrl)) continue; + if (posted === undefined || comment.createdAt < posted.createdAt) posted = comment; + } + const own = posted ? normalizePullRequestCommentAnchor(pullRequestUrlHash(posted.url)) : ""; + return own || anchor; +} + +export function pullRequestDiffPathMatches(digest: string, path: string): boolean { + const bytes = utf8ToBytes(path); + return digest === bytesToHex(digest.length === 32 ? md5(bytes) : sha256(bytes)); +} + +export function pullRequestLinkCommit(url: string | null | undefined): string | null { + if (!url) return null; + try { + return /\/pull\/\d+\/commits\/([a-f0-9]{40})\/?$/i.exec(new URL(url).pathname)?.[1] ?? null; + } catch { + return null; + } +} diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts index 8daf82277a083..68edb50064257 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts @@ -351,6 +351,7 @@ function threadComments( lastEditedAt: null, url: `https://github.com/acme/web/pull/7#discussion_${id}`, isMinimized: false, + pullRequestReview: null, reactionGroups: [], })), }; diff --git a/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts b/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts index acccedcb40f38..df72be04242b7 100644 --- a/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts +++ b/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts @@ -110,6 +110,7 @@ const remark = ({ author, reactionGroups: groups, ...raw }: Json) => ({ lastEditedAt: null, isMinimized: false, url: "https://github.com/acme/web/pull/1#discussion", + pullRequestReview: null, ...raw, author: actor(author), reactionGroups: reactionGroups(groups), @@ -893,7 +894,14 @@ describe("review thread decoding", () => { comments: { nodes: [ { id: "t1", body: "fix this", createdAt: "2026-07-01T00:00:00Z" }, - { id: "t2", body: "fixed", createdAt: "2026-07-01T01:00:00Z" }, + { + id: "t2", + body: "fixed", + createdAt: "2026-07-01T01:00:00Z", + pullRequestReview: { + url: "https://github.com/acme/web/pull/1#pullrequestreview-9", + }, + }, ], }, }, @@ -902,6 +910,11 @@ describe("review thread decoding", () => { ); const comments = reviewThreadConversation(result.threads.map((entry) => entry.thread)); expect(comments.map((comment) => comment.id)).toEqual(["t1", "t2"]); + // A reply-only review has no text of its own; its link finds it through the reply it posted. + expect(result.threads[0]?.thread.comments.map((comment) => comment.reviewUrl)).toEqual([ + undefined, + "https://github.com/acme/web/pull/1#pullrequestreview-9", + ]); }); it("leaves out hidden thread comments, and a thread with none left", () => { diff --git a/packages/source-control-github/src/server/gitHubPullRequestJson.ts b/packages/source-control-github/src/server/gitHubPullRequestJson.ts index df4756fcc0dda..f3cb11012e73f 100644 --- a/packages/source-control-github/src/server/gitHubPullRequestJson.ts +++ b/packages/source-control-github/src/server/gitHubPullRequestJson.ts @@ -1314,6 +1314,7 @@ export function toReviewDismissalsPage(result: ReviewDismissals.Result): { } function toThreadComment(comment: ThreadComment, viewer: string | null): PullRequestThreadComment { + const reviewUrl = trimmed(comment.pullRequestReview?.url); return { id: comment.id, author: toActor(comment.author), @@ -1321,6 +1322,7 @@ function toThreadComment(comment: ThreadComment, viewer: string | null): PullReq createdAt: comment.createdAt, editedAt: comment.lastEditedAt, url: trimmed(comment.url), + ...(reviewUrl === null ? {} : { reviewUrl }), reactions: toReactions(comment.reactionGroups, viewer), }; } diff --git a/packages/source-control-github/src/server/pullRequestReviews.graphql b/packages/source-control-github/src/server/pullRequestReviews.graphql index e01bcccc06a5c..d497615c24693 100644 --- a/packages/source-control-github/src/server/pullRequestReviews.graphql +++ b/packages/source-control-github/src/server/pullRequestReviews.graphql @@ -38,6 +38,10 @@ fragment ThreadComment on PullRequestReviewComment { lastEditedAt url isMinimized + # A review link names the review, not its comments; this lets it find the comments it posted. + pullRequestReview { + url + } ...Reactions } diff --git a/packages/source-control-github/src/server/pullRequestReviews.graphql.ts b/packages/source-control-github/src/server/pullRequestReviews.graphql.ts index 8eb5e0082aa12..bce1451b8c296 100644 --- a/packages/source-control-github/src/server/pullRequestReviews.graphql.ts +++ b/packages/source-control-github/src/server/pullRequestReviews.graphql.ts @@ -89,6 +89,13 @@ export const Actor = Schema.Struct({ }) export interface Actor extends Schema.Struct.Type {} +/** A review object for a given pull request. */ +export const PullRequestReviewPullRequestReview = Schema.Struct({ + /** The HTTP URL permalink for this PullRequestReview. */ + url: Shared.URI +}) +export interface PullRequestReviewPullRequestReview extends Schema.Struct.Type {} + /** * `fragment ThreadComment on PullRequestReviewComment` * @@ -109,6 +116,8 @@ export const ThreadComment = Schema.Struct({ url: Shared.URI, /** Returns whether or not a comment has been minimized. */ isMinimized: Schema.Boolean, + /** The pull request review associated with this review comment. */ + pullRequestReview: Schema.NullOr(PullRequestReviewPullRequestReview), /** A list of reactions grouped by content left on the subject. */ reactionGroups: Schema.NullOr(Schema.Array(ReactionGroup)) }) @@ -425,7 +434,7 @@ export const ReviewThreads = GraphQL.query("ReviewThreads", { fragments: [ { name: "ReviewThreadPage", body: ["on PullRequestReviewThreadConnection{totalCount pageInfo{hasNextPage endCursor}nodes{id isResolved isOutdated path line diffSide comments(first:10){totalCount pageInfo{hasNextPage endCursor}nodes{...ThreadComment}}}}"] }, { name: "Reactions", body: ["on Reactable{__typename reactionGroups{content viewerHasReacted reactors(first:10){totalCount nodes{__typename...on User{login}...on Bot{login}...on Organization{login}...on Mannequin{login}}}}}"] }, - { name: "ThreadComment", body: ["on PullRequestReviewComment{id author{__typename login avatarUrl}body createdAt lastEditedAt url isMinimized...Reactions}"] } + { name: "ThreadComment", body: ["on PullRequestReviewComment{id author{__typename login avatarUrl}body createdAt lastEditedAt url isMinimized pullRequestReview{url}...Reactions}"] } ] }, variables: { @@ -475,7 +484,7 @@ export const ReviewThreadPages = GraphQL.query("ReviewThreadPages", { ], fragments: [ { name: "ReviewThreadPage", body: ["on PullRequestReviewThreadConnection{totalCount pageInfo{hasNextPage endCursor}nodes{id isResolved isOutdated path line diffSide comments(first:10){totalCount pageInfo{hasNextPage endCursor}nodes{...ThreadComment}}}}"] }, - { name: "ThreadComment", body: ["on PullRequestReviewComment{id author{__typename login avatarUrl}body createdAt lastEditedAt url isMinimized...Reactions}"] }, + { name: "ThreadComment", body: ["on PullRequestReviewComment{id author{__typename login avatarUrl}body createdAt lastEditedAt url isMinimized pullRequestReview{url}...Reactions}"] }, { name: "Reactions", body: ["on Reactable{__typename reactionGroups{content viewerHasReacted reactors(first:10){totalCount nodes{__typename...on User{login}...on Bot{login}...on Organization{login}...on Mannequin{login}}}}}"] } ] }, @@ -552,7 +561,7 @@ export const ReviewThreadComments = GraphQL.query("ReviewThreadComments", { { name: "node", body: ["(id:$", "threadId", "){__typename...on PullRequestReviewThread{pullRequest{id}comments(first:100 after:$", "after", "){pageInfo{hasNextPage endCursor}nodes{...ThreadComment}}}}"] } ], fragments: [ - { name: "ThreadComment", body: ["on PullRequestReviewComment{id author{__typename login avatarUrl}body createdAt lastEditedAt url isMinimized...Reactions}"] }, + { name: "ThreadComment", body: ["on PullRequestReviewComment{id author{__typename login avatarUrl}body createdAt lastEditedAt url isMinimized pullRequestReview{url}...Reactions}"] }, { name: "Reactions", body: ["on Reactable{__typename reactionGroups{content viewerHasReacted reactors(first:10){totalCount nodes{__typename...on User{login}...on Bot{login}...on Organization{login}...on Mannequin{login}}}}}"] } ] },