refactor(mobile): pull request screens use a file-jump store and handler work (#25525)

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

Author: juliusmarminge

apps/mobile/src/features/diffs/nativeReviewDiffSurface.test.tsx

@@ -213,7 +213,10 @@ function setup(integrated = false) {
         focusIndex: -1,
         isIos: true,
         nativeView,
-        jumpStarted,
+        jumpStarted: () => jumpStarted.current,
+        setJumpStarted: (started) => {
+          jumpStarted.current = started;
+        },
         loadMore: () => {},
         onDebug: bridge.onDebug,
       });

apps/mobile/src/features/pull-requests/PullRequestFiles.tsx

@@ -1,12 +1,7 @@
 import { NativeScrollView } from "../../native/NativeScrollViewMarker";
 import { sourceControlClients } from "@t3tools/client-runtime/source-control-clients";
 import { AnimatedLegendList } from "@legendapp/list/reanimated";
-import {
-  StackActions,
-  useNavigation,
-  useRoute,
-  type StaticScreenProps,
-} from "@react-navigation/native";
+import { StackActions, useNavigation, type StaticScreenProps } from "@react-navigation/native";
 import type { PullRequestChangeBreakdown } from "@t3tools/contracts";
 import { useState } from "react";
 import { Platform, View } from "react-native";
@@ -36,6 +31,7 @@ import {
 } from "./usePullRequestDiff";
 import { usePullRequestFileFilter } from "./usePullRequestFileFilter";
 import { usePullRequestFilesViewed } from "./usePullRequestFilesViewed";
+import { pullRequestFileJumpScope, requestPullRequestFileJump } from "./pullRequestFileJump";
 
 const BREAKDOWN_LABELS = {
   code: "Code",
@@ -403,7 +399,7 @@ function DirectoryRow(props: {
 export function PullRequestFilesSheet({ route }: StaticScreenProps<PullRequestDiffParams>) {
   const params = route.params;
   const navigation = useNavigation();
-  const fileSelectionId = useRoute().key;
+  const jumpScope = pullRequestFileJumpScope(params);
   const files = useChangedFiles(params);
   const { diff, filter, viewed, parsed, loading } = files;
   const [collapsed, setCollapsed] = useState<ReadonlySet<string>>(() => new Set());
@@ -463,15 +459,16 @@ export function PullRequestFilesSheet({ route }: StaticScreenProps<PullRequestDi
                 <FileRow
                   file={item.file}
                   viewed={viewed.isViewed(item.file.path)}
-                  onPress={() =>
+                  onPress={() => {
+                    requestPullRequestFileJump(jumpScope, item.file.path);
                     navigation.dispatch(
                       StackActions.popTo("PullRequestFile", {
                         ...params,
                         path: item.file.path,
-                        fileSelectionId,
+                        focus: undefined,
                       }),
-                    )
-                  }
+                    );
+                  }}
                 />
               )}
             </Animated.View>

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

@@ -50,7 +50,7 @@ import { type PullRequestDetail, type PullRequestListEntry } from "@t3tools/cont
 import { PullRequestOverviewLoading } from "./PullRequestOverviewLoading";
 import { copyPullRequestLink, sharePullRequestLink } from "./pullRequestLink";
 import { pullRequestStatus } from "./pullRequestStatus";
-import { use, useEffect, useMemo, useRef, useState } from "react";
+import { use, useEffect, useEffectEvent, useMemo, useRef, useState } from "react";
 import {
   Linking,
   Platform,
@@ -123,6 +123,7 @@ import { PullRequestMergeControls } from "./PullRequestMergeControls";
 import { PullRequestStackCard } from "./PullRequestStackCard";
 import { usePullRequestStack, type PullRequestStackState } from "./usePullRequestStack";
 import { usePullRequestEnvironmentFallback } from "./usePullRequestEnvironmentFallback";
+import { pullRequestFileJumpScope, subscribePullRequestFileJump } from "./pullRequestFileJump";
 import {
   capitalizeTerm,
   sourceControlClients,
@@ -343,16 +344,26 @@ export function PullRequestRouteScreen({ route }: StaticScreenProps<PullRequestP
   const params = route.params;
   usePullRequestEnvironmentFallback(params);
   const connected = usePullRequestConnected(params.environmentId);
-  const [section, setSection] = useState<"overview" | "activity" | "guide">("overview");
-  const requestedSection = useRef(section);
+  const [chosenSection, setSection] = useState<"overview" | "activity" | "guide">("overview");
+  const requestedSection = useRef(chosenSection);
   const [listAnimationsReady, setListAnimationsReady] = useState(false);
   const [descriptionExpanded, setDescriptionExpanded] = useState(false);
   const filesContent = usePullRequestFilesSection(params);
   const guidesEnabled = useReviewGuidesEnabled(params);
+  // The guide tab goes away while guides are off and overview stands in. The pick is kept, so
+  // guides coming back (a settings reload, a reconnect) return to the guide the user chose.
+  const shown = (tab: typeof chosenSection) =>
+    !guidesEnabled && tab === "guide" ? "overview" : tab;
+  const section = shown(chosenSection);
   const [activityFilter, setActivityFilter] = useState<ActivityFilter>(DEFAULT_ACTIVITY_FILTER);
   const activityContent = usePullRequestActivityRows(params, activityFilter);
   const scrollOffsets = useRef<Record<string, number>>({});
-  const currentScrollOffset = useRef(-insets.top);
+  // The offset of the tab last shown, and which tab that was: a tab standing in for a hidden guide
+  // has its own offset, which only its next scroll would record here.
+  const currentScrollOffset = useRef<{ tab: typeof section; offset: number }>({
+    tab: "overview",
+    offset: -insets.top,
+  });
   const bodyY = useRef(0);
   const [headerHeight, setHeaderHeight] = useState(0);
   const [viewportHeight, setViewportHeight] = useState(0);
@@ -371,27 +382,20 @@ export function PullRequestRouteScreen({ route }: StaticScreenProps<PullRequestP
   });
   const selectSection = (next: typeof section) => {
     if (next === section) return;
-    scrollOffsets.current[section] = currentScrollOffset.current;
+    const from = shown(requestedSection.current);
+    const current =
+      currentScrollOffset.current.tab === from
+        ? currentScrollOffset.current.offset
+        : (scrollOffsets.current[from] ?? -insets.top);
+    scrollOffsets.current[section] = current;
     const headerEnd = Math.max(-insets.top, bodyY.current - insets.top);
-    const offset = pullRequestTabOffset(
-      currentScrollOffset.current,
-      scrollOffsets.current[next],
-      headerEnd,
-    );
+    const offset = pullRequestTabOffset(current, scrollOffsets.current[next], headerEnd);
     requestedSection.current = next;
     scrollOffsets.current[next] = offset;
-    currentScrollOffset.current = offset;
+    currentScrollOffset.current = { tab: next, offset };
     scrollViews.current[next]?.scrollToOffset({ offset, animated: false });
     setSection(next);
   };
-  useEffect(() => {
-    if (!guidesEnabled && section === "guide") {
-      requestedSection.current = "overview";
-      currentScrollOffset.current = scrollOffsets.current.overview ?? -insets.top;
-      // oxlint-disable-next-line react/set-state-in-effect -- When guides turn off, the tab and its saved scroll offset must switch to overview together.
-      setSection("overview");
-    }
-  }, [guidesEnabled, section, insets.top, currentScrollOffset, requestedSection, scrollOffsets]);
   const tabContent = activityContent;
   const filesY = useRef(0);
   const descriptionY = useRef(0);
@@ -661,7 +665,8 @@ export function PullRequestRouteScreen({ route }: StaticScreenProps<PullRequestP
                   onScroll={(event) => {
                     const offset = event.nativeEvent.contentOffset.y;
                     scrollOffsets.current[tab] = offset;
-                    if (requestedSection.current === tab) currentScrollOffset.current = offset;
+                    if (shown(requestedSection.current) === tab)
+                      currentScrollOffset.current = { tab, offset };
                   }}
                   className="flex-1"
                   renderScrollComponent={
@@ -737,7 +742,8 @@ export function PullRequestRouteScreen({ route }: StaticScreenProps<PullRequestP
                                   filesY.current = event.nativeEvent.layout.y;
                                   if (pendingFilesJump.current) {
                                     requestAnimationFrame(() => {
-                                      if (requestedSection.current === "overview") jumpToFiles();
+                                      if (shown(requestedSection.current) === "overview")
+                                        jumpToFiles();
                                     });
                                   }
                                 }}
@@ -856,7 +862,6 @@ export function PullRequestRouteScreen({ route }: StaticScreenProps<PullRequestP
 type PullRequestFileRouteProps = StaticScreenProps<
   PullRequestDiffParams & {
     readonly path: string;
-    readonly fileSelectionId?: string;
     readonly replacingDraftId?: string;
     readonly focus?: GuideDiffFocus;
   }
@@ -982,40 +987,35 @@ function PullRequestFileRouteContent({ route }: PullRequestFileRouteProps) {
   const [collapsedIds, setCollapsedIds] = useState<readonly string[]>(EMPTY_IDS);
   const [visibleFileId, setVisibleFileId] = useState<string | null>(null);
   const nativeView = useRef<NativeReviewDiffViewHandle>(null);
-  // Native applies the entry destination before its first viewport. Imperative
-  // jumps below are for subsequent selections and filter changes only.
-  const jumpStarted = useRef(true);
-  const lastSelection = useRef({ path: route.params.path, id: route.params.fileSelectionId });
-  useEffect(() => {
-    if (
-      lastSelection.current.path === route.params.path &&
-      lastSelection.current.id === route.params.fileSelectionId
-    )
-      return;
-    lastSelection.current = { path: route.params.path, id: route.params.fileSelectionId };
-    if (route.params.focus) navigation.setParams({ focus: undefined });
-    setJumpPath(route.params.path);
+  // 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<boolean | null>(filter.hideTestFiles);
+  const hasTestFiles =
+    result.parsed.kind === "files" &&
+    result.parsed.files.some((file) => isPullRequestTestFile(file.path));
+  const jumpStarted = () =>
+    jumpedUnder.current !== null && (!hasTestFiles || jumpedUnder.current === filter.hideTestFiles);
+  const scrollToFile = (fileId: string) => {
+    if (!nativeView.current) return;
+    jumpedUnder.current = filter.hideTestFiles;
+    void nativeView.current.scrollToFile(fileId, false).catch(() => {
+      jumpedUnder.current = null;
+    });
+  };
+  const jumpScope = pullRequestFileJumpScope(route.params);
+  const jumpToFile = useEffectEvent((path: string) => {
+    setJumpPath(path);
     setSelectedRowId(null);
-    jumpStarted.current = false;
-    const file = data.files.find((entry) => entry.path === route.params.path);
-    if (!file || !nativeView.current) return;
+    jumpedUnder.current = null;
+    const file = data.files.find((entry) => entry.path === path);
+    if (!file) return;
     setCollapsedIds((ids) => ids.filter((id) => id !== file.id));
-    jumpStarted.current = true;
-    void nativeView.current.scrollToFile(file.id, false).catch(() => {
-      jumpStarted.current = false;
-    });
-  }, [route.params.path, route.params.fileSelectionId, route.params.focus, data.files, navigation]);
-  const appliedFilter = useRef(filter.hideTestFiles);
-  useEffect(() => {
-    if (appliedFilter.current === filter.hideTestFiles) return;
-    appliedFilter.current = filter.hideTestFiles;
-    if (
-      result.parsed.kind === "files" &&
-      result.parsed.files.some((file) => isPullRequestTestFile(file.path))
-    ) {
-      jumpStarted.current = false;
-    }
-  }, [filter.hideTestFiles, result.parsed]);
+    scrollToFile(file.id);
+  });
+  // Subscribed for the whole screen, so a pick made while the diff is still loading is kept for
+  // the first draw.
+  useEffect(() => subscribePullRequestFileJump(jumpScope, (path) => jumpToFile(path)), [jumpScope]);
   const { nativeReviewDiffStyle } = useAppearanceCodeSurface();
   const { themeAppearance, appearance, setCodeWordBreak } = useAppearancePreferences();
   const bridge = useNativeReviewDiffBridge({
@@ -1040,6 +1040,9 @@ function PullRequestFileRouteContent({ route }: PullRequestFileRouteProps) {
       isIos: Platform.OS === "ios",
       nativeView,
       jumpStarted,
+      setJumpStarted: (started) => {
+        jumpedUnder.current = started ? filter.hideTestFiles : null;
+      },
       onDebug: bridge.onDebug,
       loadMore,
     });
@@ -1196,7 +1199,7 @@ function PullRequestFileRouteContent({ route }: PullRequestFileRouteProps) {
             onPress={() => {
               result.refresh();
               setDiffHead({ scopeKey, sha: detail.data?.headSha ?? null });
-              jumpStarted.current = false;
+              jumpedUnder.current = null;
               setSelectedRowId(null);
               setCollapsedIds(EMPTY_IDS);
             }}

apps/mobile/src/features/pull-requests/guide/PullRequestGuideScreen.tsx

@@ -207,7 +207,6 @@ export function PullRequestGuideStopScreen({ route }: StaticScreenProps<GuideSto
                             ...route.params,
                             path: excerpt.path,
                             focus: { side: excerpt.side, start: excerpt.start, end: excerpt.end },
-                            fileSelectionId: `guide:${position}:${excerpt.path}:${excerpt.start}`,
                           }),
                         )
                       }

apps/mobile/src/features/pull-requests/pullRequestDiffDebug.test.ts

@@ -35,7 +35,6 @@ it.each([true, false])(
     const scheduler = createReviewDiffHighlightScheduler(sourceRequest);
     const scrollToFile = vi.fn(async () => {});
     const loadMore = vi.fn();
-    const jumpStarted = { current: true };
     const onDebug: NonNullable<NativeReviewDiffViewProps["onDebug"]> = ({ nativeEvent }) => {
       if (nativeEvent.message === "visible-range")
         scheduler.update({
@@ -50,7 +49,8 @@ it.each([true, false])(
         focusIndex: -1,
         isIos,
         nativeView: { current: { scrollToFile, scrollToTop: vi.fn(async () => {}) } },
-        jumpStarted,
+        jumpStarted: () => true,
+        setJumpStarted: () => {},
         onDebug,
         loadMore,
       });

apps/mobile/src/features/pull-requests/pullRequestDiffDebug.ts

@@ -25,7 +25,8 @@ export function handlePullRequestDiffDebugEvent(
     readonly focusIndex: number;
     readonly isIos: boolean;
     readonly nativeView: RefObject<NativeReviewDiffViewHandle | null>;
-    readonly jumpStarted: RefObject<boolean>;
+    readonly jumpStarted: () => boolean;
+    readonly setJumpStarted: (started: boolean) => void;
     readonly onDebug: NonNullable<NativeReviewDiffViewProps["onDebug"]>;
     readonly loadMore: () => void;
   },
@@ -46,14 +47,14 @@ export function handlePullRequestDiffDebugEvent(
   if (drawn) input.onDebug(event);
   if (
     input.targetFileId &&
-    !input.jumpStarted.current &&
+    !input.jumpStarted() &&
     input.nativeView.current &&
     (!input.isIos || decoded)
   ) {
-    input.jumpStarted.current = true;
+    input.setJumpStarted(true);
     if (input.focusIndex >= 0) return;
     void input.nativeView.current.scrollToFile(input.targetFileId, false).catch(() => {
-      input.jumpStarted.current = false;
+      input.setJumpStarted(false);
     });
     return;
   }

apps/mobile/src/features/pull-requests/pullRequestFileJump.test.ts

@@ -0,0 +1,18 @@
+import { describe, expect, it } from "vite-plus/test";
+import { requestPullRequestFileJump, subscribePullRequestFileJump } from "./pullRequestFileJump";
+
+describe("pull request file jumps", () => {
+  it("delivers a pick to the screens of its scope, every time it is made", () => {
+    const seen: string[] = [];
+    const other: string[] = [];
+    const stop = subscribePullRequestFileJump("a", (path) => seen.push(path));
+    const stopOther = subscribePullRequestFileJump("b", (path) => other.push(path));
+    requestPullRequestFileJump("a", "src/x.ts");
+    requestPullRequestFileJump("a", "src/x.ts");
+    stop();
+    requestPullRequestFileJump("a", "src/y.ts");
+    stopOther();
+    expect(seen).toEqual(["src/x.ts", "src/x.ts"]);
+    expect(other).toEqual([]);
+  });
+});

apps/mobile/src/features/pull-requests/pullRequestFileJump.ts

@@ -0,0 +1,26 @@
+import type { PullRequestDiffParams } from "./usePullRequestDiff";
+import { pullRequestReviewKey } from "./pullRequestReviewDrafts";
+
+// The changed-files sheet hands its pick to the diff screen beneath it. Picking the file the
+// screen already shows still jumps back to it, which a route param that did not change could not
+// say. A screen that is not mounted yet opens at its `path` param instead.
+type Listener = (path: string) => void;
+const listeners = new Map<string, Set<Listener>>();
+
+export function pullRequestFileJumpScope(params: PullRequestDiffParams): string {
+  return JSON.stringify([pullRequestReviewKey(params), params.commit ?? null]);
+}
+
+export function requestPullRequestFileJump(scope: string, path: string): void {
+  for (const listener of listeners.get(scope) ?? []) listener(path);
+}
+
+export function subscribePullRequestFileJump(scope: string, listener: Listener): () => void {
+  let scoped = listeners.get(scope);
+  if (!scoped) listeners.set(scope, (scoped = new Set()));
+  scoped.add(listener);
+  return () => {
+    scoped.delete(listener);
+    if (scoped.size === 0) listeners.delete(scope);
+  };
+}