Skip to content

Commit b3fdf2b

Browse files
Yash-Singh1MdSadiqMdclaude
authored
fix(web): PR links jump to the comment, diff line, checks or commits they name (#33989)
Co-authored-by: Md.Sadiq <[email protected]> Co-authored-by: Claude Opus 5.5 <[email protected]>
1 parent 5c97ada commit b3fdf2b

40 files changed

Lines changed: 1871 additions & 84 deletions

‎apps/mobile/src/features/pull-requests/PullRequestDiscussionScreen.tsx‎

Lines changed: 48 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,8 @@
1+
import {
2+
parsePullRequestAnchor,
3+
pullRequestAnchorMatchesUrl,
4+
resolvePullRequestReviewAnchor,
5+
} from "@t3tools/shared/pullRequestAnchor";
16
import { AppActivityIndicator } from "../../components/ui/AppActivityIndicator";
27
import { useAtomRefresh } from "@effect/atom-react";
38
import { useFocusEffect, useNavigation, type StaticScreenProps } from "@react-navigation/native";
@@ -8,7 +13,7 @@ import {
813
import { AsyncResult } from "effect/reactivity";
914
import type { PullRequestThreadComment } from "@t3tools/contracts";
1015
import { useRef, useState } from "react";
11-
import { ScrollView, View } from "react-native";
16+
import { ScrollView, View, type ScrollViewInstance } from "react-native";
1217
import { KeyboardAvoidingView, useKeyboardState } from "react-native-keyboard-controller";
1318
import { NativeScrollViewMarker as ScrollViewMarker } from "../../native/NativeScrollViewMarker";
1419
import { useSafeAreaInsets } from "react-native-safe-area-context";
@@ -138,11 +143,27 @@ function Discussion({ params }: { params: PullRequestParams & { readonly discuss
138143
[...extra, ...(thread?.comments ?? [])].map((comment) => [comment.id, comment]),
139144
).values(),
140145
].sort((left, right) => left.createdAt.localeCompare(right.createdAt));
146+
const scrollRef = useRef<ScrollViewInstance>(null);
147+
const pendingTop = useRef<number | null>(null);
148+
const positionedComment = useRef<string | null>(null);
149+
const location = parsePullRequestAnchor(params.navigationUrl);
150+
const anchor =
151+
location?.kind === "timeline"
152+
? resolvePullRequestReviewAnchor(location.anchor, [], [{ comments }])
153+
: null;
141154
const canReply = permissions.canReply && Boolean(thread);
142155
const resolveLabel = thread?.isResolved ? "Unresolve" : "Resolve";
143156
return (
144157
<PullRequestMarkdownRepository params={params} changeRequest={detail.data}>
145-
<KeyboardAvoidingView automaticOffset behavior="height" className="flex-1 bg-screen">
158+
<KeyboardAvoidingView
159+
automaticOffset
160+
behavior="height"
161+
className="flex-1 bg-screen"
162+
// Any touch, such as replying or loading replies, ends the wait to reposition the link.
163+
onTouchStart={() => {
164+
pendingTop.current = null;
165+
}}
166+
>
146167
<AppBar
147168
title="Discussion"
148169
subtitle={`#${params.number}`}
@@ -162,6 +183,12 @@ function Discussion({ params }: { params: PullRequestParams & { readonly discuss
162183
/>
163184
<ScrollViewMarker style={{ flex: 1 }}>
164185
<ScrollView
186+
ref={scrollRef}
187+
onContentSizeChange={() => {
188+
if (pendingTop.current === null) return;
189+
scrollRef.current?.scrollTo({ y: pendingTop.current, animated: false });
190+
pendingTop.current = null;
191+
}}
165192
className="flex-1"
166193
contentInsetAdjustmentBehavior="automatic"
167194
keyboardShouldPersistTaps="handled"
@@ -192,7 +219,25 @@ function Discussion({ params }: { params: PullRequestParams & { readonly discuss
192219
<Text>This discussion is no longer available.</Text>
193220
) : null}
194221
{comments.map((comment) => (
195-
<View key={comment.id} className="gap-3 border-b border-border-subtle py-4">
222+
<View
223+
key={comment.id}
224+
onLayout={(event) => {
225+
if (
226+
anchor === null ||
227+
!pullRequestAnchorMatchesUrl(anchor, comment.url) ||
228+
positionedComment.current === comment.id
229+
)
230+
return;
231+
positionedComment.current = comment.id;
232+
pendingTop.current = event.nativeEvent.layout.y;
233+
scrollRef.current?.scrollTo({ y: pendingTop.current, animated: false });
234+
}}
235+
className={
236+
anchor !== null && pullRequestAnchorMatchesUrl(anchor, comment.url)
237+
? "-mx-3 gap-3 rounded-2xl border border-primary bg-primary/10 px-3 py-4"
238+
: "gap-3 border-b border-border-subtle py-4"
239+
}
240+
>
196241
<View className="flex-row items-center gap-2">
197242
<PullRequestAvatar actor={comment.author} size={28} />
198243
<Text className="min-w-0 flex-1 text-sm font-t3-medium">

‎apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx‎

Lines changed: 158 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,10 @@
1+
import {
2+
parsePullRequestAnchor,
3+
pullRequestAnchorMatchesUrl,
4+
pullRequestDiffPathMatches,
5+
pullRequestLinkCommit,
6+
resolvePullRequestReviewAnchor,
7+
} from "@t3tools/shared/pullRequestAnchor";
18
import { AppActivityIndicator } from "../../components/ui/AppActivityIndicator";
29
import { Chip } from "../../components/ui/Chip";
310
import { pullRequestTabOffset } from "./pullRequestTabScroll";
@@ -154,8 +161,34 @@ function usePullRequestActivityRows(params: PullRequestParams, filter: ActivityF
154161
});
155162
return rows;
156163
})();
164+
const location = parsePullRequestAnchor(params.navigationUrl);
165+
const anchor =
166+
location?.kind !== "timeline"
167+
? null
168+
: result.data
169+
? resolvePullRequestReviewAnchor(
170+
location.anchor,
171+
result.data.comments,
172+
result.data.reviewThreads,
173+
)
174+
: location.anchor;
175+
const targetComment = anchor
176+
? result.data?.comments.find((comment) => pullRequestAnchorMatchesUrl(anchor, comment.url))
177+
: null;
178+
const targetThread = anchor
179+
? result.data?.reviewThreads.find((thread) =>
180+
thread.comments.some((comment) => pullRequestAnchorMatchesUrl(anchor, comment.url)),
181+
)
182+
: null;
183+
const targetKey = targetThread
184+
? `discussion:${targetThread.id}`
185+
: targetComment
186+
? `comment:${targetComment.id}`
187+
: null;
157188
return {
158189
rows,
190+
targetKey,
191+
targetThread,
159192
hiddenCount: filtered?.hiddenCount ?? 0,
160193
pending: result.isPending && !result.data,
161194
error: result.error,
@@ -334,7 +367,10 @@ export function PullRequestRouteScreen({ route }: StaticScreenProps<PullRequestP
334367
const params = route.params;
335368
usePullRequestEnvironmentFallback(params);
336369
const connected = usePullRequestConnected(params.environmentId);
337-
const [chosenSection, setSection] = useState<"overview" | "activity" | "guide">("overview");
370+
const [chosenSection, setSection] = useState<"overview" | "activity" | "guide">(() => {
371+
const kind = parsePullRequestAnchor(params.navigationUrl)?.kind;
372+
return kind === "timeline" || kind === "commits" ? "activity" : "overview";
373+
});
338374
const requestedSection = useRef(chosenSection);
339375
const [listAnimationsReady, setListAnimationsReady] = useState(false);
340376
const [descriptionExpanded, setDescriptionExpanded] = useState(false);
@@ -345,7 +381,14 @@ export function PullRequestRouteScreen({ route }: StaticScreenProps<PullRequestP
345381
const shown = (tab: typeof chosenSection) =>
346382
!guidesEnabled && tab === "guide" ? "overview" : tab;
347383
const section = shown(chosenSection);
348-
const [activityFilter, setActivityFilter] = useState<ActivityFilter>(DEFAULT_ACTIVITY_FILTER);
384+
const [activityFilter, setActivityFilter] = useState<ActivityFilter>(() => {
385+
const kind = parsePullRequestAnchor(params.navigationUrl)?.kind;
386+
return kind === "timeline"
387+
? { ...DEFAULT_ACTIVITY_FILTER, bots: true, resolved: true }
388+
: kind === "commits"
389+
? { ...DEFAULT_ACTIVITY_FILTER, kind: "commits" }
390+
: DEFAULT_ACTIVITY_FILTER;
391+
});
349392
const activityContent = usePullRequestActivityRows(params, activityFilter);
350393
const scrollOffsets = useRef<Record<string, number>>({});
351394
// 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<PullRequestP
386429
scrollViews.current[next]?.scrollToOffset({ offset, animated: false });
387430
setSection(next);
388431
};
432+
const followLink = useEffectEvent(() => {
433+
const target = parsePullRequestAnchor(params.navigationUrl);
434+
if (target?.kind === "diff" || target?.kind === "code") {
435+
const commit = pullRequestLinkCommit(params.navigationUrl);
436+
navigation.navigate("PullRequestFile", {
437+
...params,
438+
path: "",
439+
...(commit ? { commit } : {}),
440+
});
441+
} else if (target?.kind === "timeline") {
442+
selectSection("activity");
443+
setActivityFilter((filter) => ({ ...filter, kind: "all", bots: true, resolved: true }));
444+
} else if (target?.kind === "commits") {
445+
selectSection("activity");
446+
setActivityFilter((filter) => ({ ...filter, kind: "commits" }));
447+
} else if (target?.kind === "checks") {
448+
selectSection("overview");
449+
// Checks head Overview, which a reused screen may have scrolled past.
450+
scrollViews.current.overview?.scrollToOffset({ offset: -contentTopInset, animated: false });
451+
} else if (target?.kind === "summary") {
452+
selectSection("overview");
453+
}
454+
});
455+
// oxlint-disable-next-line t3code/no-unreasoned-state-sync -- a link selects its native PR section or diff even when the route is reused
456+
useEffect(() => {
457+
// oxlint-disable-next-line react/set-state-in-effect -- URL navigation selects the PR section and makes its target visible
458+
followLink();
459+
// oxlint-disable-next-line react/exhaustive-effect-dependencies -- each clicked URL is a fresh navigation request
460+
}, [params.navigationUrl, params.navigationId]);
461+
462+
const activityTargetIndex = activityContent.rows.findIndex(
463+
(row) => row.key === activityContent.targetKey,
464+
);
465+
const openedDiscussion = useRef<string | null>(null);
466+
const positionedActivity = useRef<string | null>(null);
467+
// Rows above the target measure in after the first jump, so it is held until the user scrolls.
468+
const heldActivityRow = useRef<number | null>(null);
469+
const positionActivityRow = (index: number) =>
470+
void scrollViews.current.activity?.scrollToIndex({
471+
index,
472+
viewPosition: 0,
473+
viewOffset: contentTopInset + 12,
474+
animated: false,
475+
});
476+
const revealActivity = useEffectEvent(() => {
477+
const key = `${params.navigationUrl}:${params.navigationId ?? 0}`;
478+
if (activityContent.targetThread && openedDiscussion.current !== key) {
479+
openedDiscussion.current = key;
480+
navigation.navigate("PullRequestDiscussion", {
481+
...params,
482+
discussionId: activityContent.targetThread.id,
483+
});
484+
return;
485+
}
486+
// Later filter or refresh changes move the row; they must not pull the user back to it.
487+
if (activityTargetIndex >= 0 && positionedActivity.current !== key) {
488+
positionedActivity.current = key;
489+
// Its top sits under the app bar; centering a long comment pushed its start off screen.
490+
heldActivityRow.current = activityTargetIndex + 1;
491+
positionActivityRow(activityTargetIndex + 1);
492+
}
493+
});
494+
const targetThreadId = activityContent.targetThread?.id;
495+
// oxlint-disable-next-line t3code/no-unreasoned-state-sync -- reveals the requested activity once its asynchronous rows arrive
496+
useEffect(() => {
497+
revealActivity();
498+
// oxlint-disable-next-line react/exhaustive-effect-dependencies -- reruns when activity resolves the URL target, even a thread the filter hides
499+
}, [params.navigationUrl, params.navigationId, activityTargetIndex, targetThreadId]);
389500
const tabContent = activityContent;
390501
const filesY = useRef(0);
391502
const descriptionY = useRef(0);
@@ -596,6 +707,13 @@ export function PullRequestRouteScreen({ route }: StaticScreenProps<PullRequestP
596707
// The scroll view owns its resting offset. LegendList keeps the first
597708
// initialScrollOffset it gets, and the first render has the window's top
598709
// inset (0 on the folded iPhone Duo), not the header's.
710+
onScrollBeginDrag={
711+
tab === "activity"
712+
? () => {
713+
heldActivityRow.current = null;
714+
}
715+
: undefined
716+
}
599717
onScroll={(event) => {
600718
const offset = event.nativeEvent.contentOffset.y;
601719
scrollOffsets.current[tab] = offset;
@@ -619,6 +737,13 @@ export function PullRequestRouteScreen({ route }: StaticScreenProps<PullRequestP
619737
alwaysBounceVertical
620738
scrollsToTop={section === tab}
621739
onItemSizeChanged={({ itemKey }) => {
740+
const held = heldActivityRow.current;
741+
if (tab === "activity" && held !== null) {
742+
// A filter or refresh that moved the row ends the hold.
743+
if (held !== activityTargetIndex + 1) heldActivityRow.current = null;
744+
else positionActivityRow(held);
745+
return;
746+
}
622747
if (
623748
tab !== "overview" ||
624749
section !== "overview" ||
@@ -848,9 +973,20 @@ function PullRequestFileRouteContent({ route }: PullRequestFileRouteProps) {
848973
(detail.data ?? route.params.initialSummary)?.provider,
849974
).changeRequest;
850975
const result = usePullRequestDiff(route.params, terminology);
976+
// Other screens open files with the link's params spread in; only the link's own entry follows it.
977+
const linkAnchor =
978+
route.params.path === "" &&
979+
(route.params.commit ?? null) === pullRequestLinkCommit(route.params.navigationUrl)
980+
? parsePullRequestAnchor(route.params.navigationUrl)
981+
: null;
982+
const linkPath =
983+
linkAnchor?.kind === "diff" && result.parsed.kind === "files"
984+
? result.parsed.files.find((file) => pullRequestDiffPathMatches(linkAnchor.digest, file.path))
985+
?.path
986+
: undefined;
851987
const filter = usePullRequestFileFilter(
852988
result.parsed,
853-
route.params.focus ? route.params.path : undefined,
989+
linkPath ?? (route.params.focus ? route.params.path : undefined),
854990
);
855991
const { refresh: refreshDetail } = detail;
856992
useFocusEffect(refreshDetail);
@@ -922,15 +1058,30 @@ function PullRequestFileRouteContent({ route }: PullRequestFileRouteProps) {
9221058
code: row.content ?? "",
9231059
});
9241060
};
925-
const [jumpPath, setJumpPath] = useState(route.params.path);
1061+
const [pickedPath, setJumpPath] = useState<string | null>(null);
1062+
const jumpPath = pickedPath ?? linkPath ?? route.params.path;
9261063
const requestedFile =
9271064
result.parsed.kind === "files"
9281065
? result.parsed.files.find((file) => file.path === jumpPath)
9291066
: undefined;
9301067
const targetFile =
9311068
data.files.find((file) => file.path === jumpPath) ??
932-
(!jumpPath || requestedFile ? data.files[0] : undefined);
933-
const focus = route.params.focus;
1069+
(linkAnchor?.kind === "diff" && !linkPath
1070+
? undefined
1071+
: !jumpPath || requestedFile
1072+
? data.files[0]
1073+
: undefined);
1074+
const range = pickedPath === null && linkAnchor?.kind === "diff" ? linkAnchor.range : null;
1075+
const focus =
1076+
route.params.focus ??
1077+
(range
1078+
? {
1079+
side: range.side === "left" ? ("old" as const) : ("new" as const),
1080+
start: range.start,
1081+
end: range.end,
1082+
endSide: range.endSide === "left" ? ("old" as const) : ("new" as const),
1083+
}
1084+
: undefined);
9341085
const focusedRows = guideFocusedRows(data.rows, targetFile?.id, focus);
9351086
const focusIndex = focusedRows.length
9361087
? data.rows.findIndex((row) => row.id === focusedRows[0]?.id)
@@ -941,7 +1092,7 @@ function PullRequestFileRouteContent({ route }: PullRequestFileRouteProps) {
9411092
// The filter the current jump was made under. Hiding or showing test files replaces the rows,
9421093
// so a jump made under the other filter has to run again. Native applies the entry
9431094
// destination before its first viewport, so the first one counts as made.
944-
const jumpedUnder = useRef<boolean | null>(filter.hideTestFiles);
1095+
const jumpedUnder = useRef<boolean | null>(targetFile ? filter.hideTestFiles : null);
9451096
const hasTestFiles =
9461097
result.parsed.kind === "files" &&
9471098
result.parsed.files.some((file) => isTestChangedFile(file.path));

‎apps/mobile/src/features/pull-requests/PullRequestStatusRows.tsx‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { parsePullRequestAnchor } from "@t3tools/shared/pullRequestAnchor";
12
import { usePullRequestConnected } from "./usePullRequestConnected";
23
import { useAtomValue } from "@effect/atom-react";
34
import { useNavigation } from "@react-navigation/native";
@@ -102,7 +103,13 @@ export function PullRequestStatusMenu(props: RowProps) {
102103
}
103104

104105
export function PullRequestChecksRow(props: RowProps) {
105-
const [expanded, setExpanded] = useState(false);
106+
const linked = parsePullRequestAnchor(props.params.navigationUrl)?.kind === "checks";
107+
const [expanded, setExpanded] = useState(linked);
108+
const [seenNavigation, setSeenNavigation] = useState(props.params.navigationId);
109+
if (props.params.navigationId !== seenNavigation) {
110+
setSeenNavigation(props.params.navigationId);
111+
if (linked) setExpanded(true);
112+
}
106113
const checks = pullRequestChecksSummary(props.pr);
107114
const action = usePullRequestAction(props.params, props.pr);
108115
const definition = sourceControlClients.get(props.pr.provider);

0 commit comments

Comments
 (0)