From 975c1e361cb77fffe9209ffcd8df5936a98a10aa Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 26 Oct 2026 23:36:10 +1100 Subject: [PATCH 1/2] fix(mobile): stop refreshes in hidden PR Guide panes --- .../pull-requests/PullRequestRouteScreen.tsx | 7 +- .../guide/PullRequestGuideScreen.tsx | 10 +- .../guide/PullRequestGuideVisibility.test.tsx | 446 ++++++++++++++++++ .../guide/useReviewGuide.test.tsx | 130 ++++- .../pull-requests/guide/useReviewGuide.ts | 4 +- apps/mobile/vite.config.ts | 1 + 6 files changed, 584 insertions(+), 14 deletions(-) create mode 100644 apps/mobile/src/features/pull-requests/guide/PullRequestGuideVisibility.test.tsx diff --git a/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx b/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx index 3ceba8273c4ba..bdf47d773a3e5 100644 --- a/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx +++ b/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx @@ -746,7 +746,12 @@ export function PullRequestRouteScreen({ route }: StaticScreenProps ) : null} - {tab === "guide" ? : null} + {tab === "guide" ? ( + + ) : null} {tab === "activity" ? ( diff --git a/apps/mobile/src/features/pull-requests/guide/PullRequestGuideVisibility.test.tsx b/apps/mobile/src/features/pull-requests/guide/PullRequestGuideVisibility.test.tsx new file mode 100644 index 0000000000000..c4d20e0786db6 --- /dev/null +++ b/apps/mobile/src/features/pull-requests/guide/PullRequestGuideVisibility.test.tsx @@ -0,0 +1,446 @@ +import { + ProviderInstanceId, + type PullRequestListEntry, + type PullRequestReviewGuideResult, +} from "@t3tools/contracts"; +import { RegistryContext } from "@effect/atom-react"; +import { + NavigationContext, + type NavigationProp, + type ParamListBase, +} from "@react-navigation/native"; +import { AsyncResult, Atom, AtomRegistry } from "effect/reactivity"; +import { act, type ReactNode } from "react"; +import { createRoot } from "react-dom/client"; +import { expect, it, vi } from "vite-plus/test"; + +const mocks = vi.hoisted(() => ({ + guide: null as unknown, + settings: null as unknown, + permission: null as unknown, + empty: null as unknown, + refresh: vi.fn(), +})); + +vi.mock("@effect/atom-react", async (importOriginal) => { + const actual = await importOriginal(); + const { useMemo } = await import("react"); + return { + ...actual, + useAtomRefresh: (atom: Parameters[0]) => { + const refresh = actual.useAtomRefresh(atom); + return useMemo( + () => () => { + if (atom === mocks.guide) mocks.refresh(); + refresh(); + }, + [atom, refresh], + ); + }, + }; +}); +vi.mock("@react-navigation/native", async () => { + const { createRequire } = await import("node:module"); + const { realpathSync } = await import("node:fs"); + const require = createRequire(import.meta.url); + const nativeRequire = createRequire( + realpathSync(require.resolve("@react-navigation/native/package.json")), + ); + const core: typeof import("@react-navigation/native") = nativeRequire("@react-navigation/core"); + return { + NavigationContext: core.NavigationContext, + useFocusEffect: core.useFocusEffect, + useNavigation: core.useNavigation, + StackActions: core.StackActions, + }; +}); +vi.mock("react-native", () => { + const View = ({ + children, + accessibilityElementsHidden, + }: { + children?: ReactNode; + accessibilityElementsHidden?: boolean; + }) => ; + return { + View, + ScrollView: View, + Pressable: View, + RefreshControl: () => null, + Platform: { OS: "ios" }, + StyleSheet: { absoluteFill: {} }, + }; +}); +vi.mock("react-native-reanimated", () => ({ + default: { View: ({ children }: { children?: ReactNode }) =>
{children}
}, + FadeIn: { delay: () => ({ duration: () => ({ reduceMotion: () => ({}) }) }) }, + ReduceMotion: { System: "system" }, +})); +vi.mock("@legendapp/list/reanimated", () => ({ + // Render retained rows across section changes; native list virtualization is parent-owned proof. + AnimatedLegendList: ({ + data, + renderItem, + ListHeaderComponent, + }: { + data: { key: string; content: ReactNode }[]; + renderItem: (row: { item: { key: string; content: ReactNode } }) => ReactNode; + ListHeaderComponent: ReactNode; + }) => ( +
+ {ListHeaderComponent} + {data.map((item) => ( +
{renderItem({ item })}
+ ))} +
+ ), +})); +vi.mock("react-native-safe-area-context", () => ({ + useSafeAreaInsets: () => ({ top: 0, bottom: 0 }), +})); +vi.mock("expo-clipboard", () => ({})); +vi.mock("../pullRequestLink", () => ({ + copyPullRequestLink: () => {}, + sharePullRequestLink: () => {}, +})); +vi.mock("@pierre/diffs/utils/parsePatchFiles", () => ({ parsePatchFiles: () => [] })); +vi.mock("@t3tools/client-runtime/source-control-clients", () => ({ + sourceControlClients: { get: () => ({ changeRequest: { singular: "pull request" } }) }, + capitalizeTerm: (term: string) => term, +})); +vi.mock("../../../state/pull-requests", () => ({ + composerPullRequests: { + reviewGuide: () => mocks.guide, + detail: () => mocks.empty, + activity: () => mocks.empty, + generateReviewGuide: { permissionAtom: () => mocks.permission }, + }, +})); +vi.mock("../../../state/server", () => ({ + serverEnvironment: { settingsValueAtom: () => mocks.settings }, +})); +vi.mock("../../../state/use-atom-command", () => ({ useAtomCommand: () => vi.fn() })); +vi.mock("../../../state/use-pull-to-refresh", () => ({ + usePullToRefresh: () => ({ refreshing: false }), +})); +vi.mock("../../../components/ui/Toolbar", () => { + const Empty = () => null; + return { Toolbar: Object.assign(Empty, { Menu: Empty, MenuAction: Empty, Label: Empty }) }; +}); +vi.mock("../../../components/ui/AppText", () => ({ + AppText: ({ children }: { children?: ReactNode }) => {children}, +})); +vi.mock("../../../components/ui/Button", () => ({ + Button: ({ + label, + disabled, + onPress, + }: { + label: string; + disabled?: boolean; + onPress: () => void; + }) => ( + + ), +})); +vi.mock("../../../components/ui/SegmentedControl", () => ({ + SegmentedControl: ({ + options, + onSelect, + }: { + options: { value: string; label: string }[]; + onSelect: (value: string) => void; + }) => ( + + ), +})); +vi.mock("../../../components/ui/ScreenScrollView", () => ({ + ScreenScrollView: ({ children }: { children?: ReactNode }) =>
{children}
, +})); +vi.mock("../../../native/StackHeader", () => ({ NativeStackScreenOptions: () => null })); +vi.mock("../../../native/NativeScrollViewMarker", () => ({ + NativeScrollViewMarker: ({ children }: { children?: ReactNode }) =>
{children}
, +})); +vi.mock("../../../components/ui/useAndroidControlSizing", () => ({ + useAndroidControlSizing: () => ({ buttonSize: 32 }), +})); +vi.mock("../../layout/AdaptiveWorkspaceLayout", () => ({ + useAdaptiveWorkspaceLayout: () => ({ layout: { usesSplitView: false } }), +})); +vi.mock("../../layout/useWorkspaceSidebarHeaderItem", () => ({ + useWorkspaceSidebarHeaderItem: () => null, +})); +vi.mock("../../workspace/useAndroidWorkspaceNavigation", () => ({ + useAndroidWorkspaceBack: () => vi.fn(), +})); +vi.mock("../usePullRequestConnected", () => ({ usePullRequestConnected: () => true })); +vi.mock("../usePullRequestLiveChecks", () => ({ usePullRequestLiveChecks: () => null })); +vi.mock("../usePullRequestEnvironmentFallback", () => ({ + usePullRequestEnvironmentFallback: () => null, +})); +vi.mock("../usePullRequestPermissions", () => ({ + usePullRequestPermissions: () => ({ verdicts: [] }), +})); +vi.mock("../usePullRequestAction", () => ({ usePullRequestAction: () => ({}) })); +vi.mock("../usePullRequestStack", () => ({ usePullRequestStack: () => ({}) })); +vi.mock("../PullRequestFiles", () => ({ usePullRequestFilesSection: () => ({ rows: [] }) })); +vi.mock("../PullRequestTimeline", () => ({ pullRequestTimelineRows: () => [] })); +vi.mock("../PullRequestMarkdown", () => ({ + PullRequestMarkdownRepository: ({ children }: { children?: ReactNode }) => <>{children}, + PullRequestMarkdown: () => null, +})); +vi.mock("../PullRequestMedia", async () => ({ + PullRequestMediaVisibleContext: (await import("react")).createContext(true), +})); +vi.mock("../usePullRequestDiff", () => ({ + pullRequestTarget: (params: unknown) => params, + usePullRequestDiff: () => ({ patch: "", parsed: { kind: "files", files: [] } }), +})); +vi.mock("../../settings/appearance/AppearancePreferencesProvider", () => ({ + useAppearancePreferences: () => ({ appearance: {} }), +})); +vi.mock("./GuideOutline", () => ({ + GuideOutline: ({ guide }: { guide: { headSha: string } }) =>

Cached guide {guide.headSha}

, +})); +vi.mock("../../../components/ui/AppActivityIndicator", () => ({ + AppActivityIndicator: () => null, +})); +vi.mock("../../../components/ui/Chip", () => ({ Chip: () => null })); +vi.mock("../../../components/ui/Menu", () => ({ Menu: () => null })); +vi.mock("../../../components/ui/IconButton", () => ({ IconButton: () => null })); +vi.mock("../../../components/ui/AndroidScreenHeader", () => ({ AndroidScreenHeader: () => null })); +vi.mock("../../../components/ui/EmptyState", () => ({ EmptyState: () => null })); +vi.mock("../../../components/ui/AppSymbol", () => ({ SymbolView: () => null })); +vi.mock("../../layout/workspace-sidebar-toolbar", () => ({ + AndroidWorkspaceSidebarButton: () => null, +})); +vi.mock("../PullRequestActivityFilters", () => ({ PullRequestActivityFilters: () => null })); +vi.mock("../PullRequestOverviewLoading", () => ({ PullRequestOverviewLoading: () => null })); +vi.mock("../PullRequestPresentation", () => ({ + PullRequestAvatar: () => null, + PullRequestDiffStats: () => null, + PullRequestReadStatus: () => null, +})); +vi.mock("../PullRequestControls", () => ({ + PullRequestActionToolbar: () => null, + PullRequestCardGroup: () => null, + PullRequestCardRow: () => null, +})); +vi.mock("../PullRequestStatusRows", () => ({ + PullRequestStatusMenu: () => null, + PullRequestChecksRow: () => null, + PullRequestBranchRow: () => null, + PullRequestReviewersRow: () => null, +})); +vi.mock("../PullRequestMergeControls", () => ({ PullRequestMergeControls: () => null })); +vi.mock("../PullRequestStackCard", () => ({ PullRequestStackCard: () => null })); +vi.mock("../PullRequestReactions", () => ({ PullRequestReactions: () => null })); +vi.mock("../PullRequestDescription", () => ({ PullRequestDescription: () => null })); +vi.mock("./ReviewGuideExcerpt", () => ({ ReviewGuideExcerpt: () => null })); +vi.mock("../usePullRequestReviewDraft", () => ({ usePullRequestReviewDraft: () => null })); +vi.mock("../usePullRequestFilesViewed", () => ({ usePullRequestFilesViewed: () => null })); +vi.mock("../usePullRequestFileFilter", () => ({ usePullRequestFileFilter: () => null })); +vi.mock("../usePullRequestDiffHighlightFiles", () => ({ + usePullRequestDiffHighlightFiles: () => null, +})); +vi.mock("../../settings/appearance/useAppearanceCodeSurface", () => ({ + useAppearanceCodeSurface: () => null, +})); +vi.mock("../../review/useNativeReviewDiffBridge", () => ({ + useNativeReviewDiffBridge: () => null, +})); +vi.mock("../../diffs/nativeReviewDiffSurface", () => ({ resolveNativeReviewDiffView: () => null })); +vi.mock("../../review/nativeReviewDiffAdapter", () => ({ + getCachedNativeReviewDiffData: () => null, +})); +vi.mock("../pullRequestAnimations", () => ({ + PR_ROW_LAYOUT: {}, + PR_ROW_ENTER: {}, + PR_ROW_EXIT: {}, + PR_POSITION_LAYOUT: {}, +})); + +import { PullRequestRouteScreen } from "../PullRequestRouteScreen"; +import { PullRequestGuideStopScreen } from "./PullRequestGuideScreen"; + +function setup() { + (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + mocks.refresh = vi.fn(); + const initial: PullRequestReviewGuideResult = { + currentHeadSha: "cached-head", + guide: { + headSha: "cached-head", + generatedAt: "2026-10-26T00:00:00Z", + modelSelection: { instanceId: ProviderInstanceId.make("codex"), model: "gpt-6.1-sol" }, + stops: [], + }, + run: { _tag: "Running", headSha: "cached-head", startedAt: "2026-10-26T00:00:00Z" }, + }; + const source = Atom.make(AsyncResult.success(initial)); + let acquisitions = 0; + let releases = 0; + const guide = Atom.make((get) => { + acquisitions++; + get.addFinalizer(() => { + releases++; + }); + return get(source); + }).pipe(Atom.setIdleTTL("0 millis")); + mocks.guide = guide; + mocks.empty = Atom.make(AsyncResult.success(null)); + mocks.settings = Atom.make({ reviewGuidesEnabled: true }); + mocks.permission = Atom.make(true); + const registry = AtomRegistry.make(); + const container = document.createElement("div"); + const root = createRoot(container); + const listeners = { focus: new Set<() => void>(), blur: new Set<() => void>() }; + let focused = true; + const navigation = { + isFocused: () => focused, + addListener: (event: keyof typeof listeners, listener: () => void) => { + listeners[event].add(listener); + return () => listeners[event].delete(listener); + }, + } as unknown as NavigationProp; + const params = { + environmentId: "environment", + projectId: "project", + repository: "repo", + number: 1, + initialSummary: { + title: "Test PR", + state: "open", + isDraft: false, + provider: "github", + updatedAt: "2026-10-26T00:00:00Z", + author: null, + baseBranch: "main", + headBranch: "change", + } as PullRequestListEntry, + }; + return { + container, + render(stop = false) { + act(() => + root.render( + + + + {stop ? ( + + ) : null} + + , + ), + ); + }, + select(label: string) { + const button = [...container.querySelectorAll("button")].find( + (item) => item.textContent === label, + ); + if (!button) throw new Error(`Missing section ${label}`); + act(() => button.click()); + }, + complete() { + act(() => registry.set(source, AsyncResult.success({ ...initial, run: null }))); + }, + focus(next: boolean) { + focused = next; + act(() => { + for (const listener of listeners[next ? "focus" : "blur"]) listener(); + }); + }, + get acquisitions() { + return acquisitions; + }, + get releases() { + return releases; + }, + [Symbol.dispose]() { + act(() => root.unmount()); + registry.dispose(); + expect(releases).toBe(acquisitions); + expect(listeners.focus.size).toBe(0); + expect(listeners.blur.size).toBe(0); + }, + }; +} + +it("retains the production Guide query and cached outline while hidden, then refreshes once on reveal", () => { + vi.useFakeTimers(); + using _timers = { [Symbol.dispose]: () => vi.useRealTimers() }; + { + using screen = setup(); + screen.render(); + screen.select("Guide"); + const outline = screen.container.querySelector("p"); + expect(outline?.textContent).toBe("Cached guide cached-head"); + expect(screen.container.textContent).toContain("Rewriting guide"); + screen.select("Overview"); + const beforeHidden = mocks.refresh.mock.calls.length; + const acquisitions = screen.acquisitions; + const releases = screen.releases; + act(() => vi.advanceTimersByTime(60_000)); + expect({ + manualRefreshes: mocks.refresh.mock.calls.length - beforeHidden, + queryAcquisitions: screen.acquisitions - acquisitions, + }).toEqual({ manualRefreshes: 0, queryAcquisitions: 0 }); + expect(screen.releases).toBe(releases); + expect(screen.container.querySelector("p")).toBe(outline); + + screen.complete(); + expect(screen.container.textContent).not.toContain("Rewriting guide"); + expect(mocks.refresh).toHaveBeenCalledTimes(beforeHidden); + expect(screen.acquisitions).toBeGreaterThan(acquisitions); + screen.select("Guide"); + expect(mocks.refresh).toHaveBeenCalledTimes(beforeHidden + 1); + expect(screen.container.querySelector("p")).toBe(outline); + act(() => vi.advanceTimersByTime(10_000)); + expect(mocks.refresh).toHaveBeenCalledTimes(beforeHidden + 1); + } + // AtomRegistry defers node removal to its zero-delay dispatcher. + act(() => vi.advanceTimersByTime(0)); + expect(vi.getTimerCount()).toBe(0); +}); + +it("keeps the standalone GuideStop reader refreshing while the retained pane is hidden", () => { + vi.useFakeTimers(); + using _timers = { [Symbol.dispose]: () => vi.useRealTimers() }; + { + using screen = setup(); + screen.render(true); + const initial = mocks.refresh.mock.calls.length; + act(() => vi.advanceTimersByTime(5_000)); + expect(mocks.refresh).toHaveBeenCalledTimes(initial + 1); + screen.focus(false); + act(() => vi.advanceTimersByTime(5_000)); + expect(mocks.refresh).toHaveBeenCalledTimes(initial + 1); + screen.focus(true); + expect(mocks.refresh).toHaveBeenCalledTimes(initial + 2); + screen.render(false); + const afterStop = mocks.refresh.mock.calls.length; + act(() => vi.advanceTimersByTime(60_000)); + expect(mocks.refresh).toHaveBeenCalledTimes(afterStop); + expect(vi.getTimerCount()).toBe(0); + } + // AtomRegistry defers node removal to its zero-delay dispatcher. + act(() => vi.advanceTimersByTime(0)); + expect(vi.getTimerCount()).toBe(0); +}); diff --git a/apps/mobile/src/features/pull-requests/guide/useReviewGuide.test.tsx b/apps/mobile/src/features/pull-requests/guide/useReviewGuide.test.tsx index 764cd5f881ac9..e797a12a57217 100644 --- a/apps/mobile/src/features/pull-requests/guide/useReviewGuide.test.tsx +++ b/apps/mobile/src/features/pull-requests/guide/useReviewGuide.test.tsx @@ -3,18 +3,21 @@ import { type NavigationProp, type ParamListBase, } from "@react-navigation/native"; -import { act } from "react"; +import { AsyncResult } from "effect/reactivity"; +import { act, useLayoutEffect } from "react"; import { createRoot } from "react-dom/client"; import { expect, it, vi } from "vite-plus/test"; const mocks = vi.hoisted(() => ({ + enabled: true, + generate: vi.fn(), query: { - data: null as { run: { _tag: "Running" | "Completed" } } | null, + data: null as { run: { _tag: "Running" | "Completed" | "Failed" } | null } | null, isPending: false, refresh: vi.fn<() => void>(), }, otherQuery: { - data: null as { run: { _tag: "Running" | "Completed" } } | null, + data: null as { run: { _tag: "Running" | "Completed" | "Failed" } | null } | null, isPending: false, refresh: vi.fn<() => void>(), }, @@ -33,7 +36,8 @@ vi.mock("@react-navigation/native", async () => { return { NavigationContext, useFocusEffect }; }); vi.mock("@effect/atom-react", () => ({ - useAtomValue: (atom: unknown) => (atom === "settings" ? { reviewGuidesEnabled: true } : true), + useAtomValue: (atom: unknown) => + atom === "settings" ? { reviewGuidesEnabled: mocks.enabled } : true, })); vi.mock("../../../state/server", () => ({ serverEnvironment: { settingsValueAtom: () => "settings" }, @@ -48,7 +52,7 @@ vi.mock("../../../state/pull-requests", () => ({ vi.mock("../../../state/query", () => ({ useEnvironmentQuery: (number: number) => ({ ...(number === 1 ? mocks.query : mocks.otherQuery) }), })); -vi.mock("../../../state/use-atom-command", () => ({ useAtomCommand: () => vi.fn() })); +vi.mock("../../../state/use-atom-command", () => ({ useAtomCommand: () => mocks.generate })); vi.mock("../usePullRequestConnected", () => ({ usePullRequestConnected: () => true })); vi.mock("../usePullRequestDiff", () => ({ pullRequestTarget: (params: unknown) => params, @@ -78,6 +82,8 @@ function createNavigation() { function setup() { (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + mocks.enabled = true; + mocks.generate = vi.fn().mockResolvedValue(AsyncResult.success(undefined)); mocks.query = { data: null, isPending: false, refresh: vi.fn() }; mocks.otherQuery = { data: null, isPending: false, refresh: vi.fn() }; const root = createRoot(document.createElement("div")); @@ -88,20 +94,35 @@ function setup() { repository: "repo", number: 1, }; - function Probe({ number, environmentId }: { number: number; environmentId: string }) { - useReviewGuide({ ...params, number, environmentId }); + let current: ReturnType; + function Probe({ + number, + environmentId, + visible, + }: { + number: number; + environmentId: string; + visible?: boolean; + }) { + const state = useReviewGuide({ ...params, number, environmentId }, visible); + useLayoutEffect(() => { + current = state; + }); return null; } return { - render(number = 1, environmentId = params.environmentId) { + render(number = 1, environmentId = params.environmentId, visible?: boolean) { act(() => root.render( - + , ), ); }, + get current() { + return current; + }, focus: navigation.focus, [Symbol.dispose]() { act(() => root.unmount()); @@ -188,3 +209,94 @@ it("retires the old environment's polling on target change and clears polling on expect(mocks.otherQuery.refresh).toHaveBeenCalledTimes(2); expect(vi.getTimerCount()).toBe(0); }); + +it("suspends only manual refresh while hidden, including focus and target changes", () => { + vi.useFakeTimers(); + using _timers = { [Symbol.dispose]: () => vi.useRealTimers() }; + { + using screen = setup(); + mocks.query.data = { run: { _tag: "Running" } }; + mocks.otherQuery.data = { run: { _tag: "Running" } }; + screen.render(); + const cached = screen.current.data; + screen.render(1, "environment", false); + act(() => vi.advanceTimersByTime(60_000)); + expect(mocks.query.refresh).toHaveBeenCalledTimes(1); + expect(screen.current.data).toBe(cached); + screen.render(); + expect(mocks.query.refresh).toHaveBeenCalledTimes(2); + act(() => vi.advanceTimersByTime(5_000)); + expect(mocks.query.refresh).toHaveBeenCalledTimes(3); + screen.render(1, "environment", false); + screen.focus(false); + screen.focus(true); + expect(mocks.query.refresh).toHaveBeenCalledTimes(3); + + screen.render(2, "other-environment", false); + act(() => vi.advanceTimersByTime(5_000)); + expect(mocks.otherQuery.refresh).not.toHaveBeenCalled(); + screen.focus(false); + screen.render(2, "other-environment", true); + expect(mocks.otherQuery.refresh).not.toHaveBeenCalled(); + screen.focus(true); + expect(mocks.otherQuery.refresh).toHaveBeenCalledTimes(1); + act(() => vi.advanceTimersByTime(5_000)); + expect(mocks.otherQuery.refresh).toHaveBeenCalledTimes(2); + expect(mocks.query.refresh).toHaveBeenCalledTimes(3); + } + expect(vi.getTimerCount()).toBe(0); +}); + +it("keeps disabled, completed and failed guides idle and refreshes once on reveal", () => { + vi.useFakeTimers(); + using _timers = { [Symbol.dispose]: () => vi.useRealTimers() }; + using screen = setup(); + mocks.enabled = false; + screen.render(); + act(() => vi.advanceTimersByTime(5_000)); + expect(mocks.query.refresh).not.toHaveBeenCalled(); + mocks.enabled = true; + mocks.query.data = { run: null }; + screen.render(); + expect(mocks.query.refresh).toHaveBeenCalledTimes(1); + act(() => vi.advanceTimersByTime(10_000)); + expect(mocks.query.refresh).toHaveBeenCalledTimes(1); + screen.render(1, "environment", false); + mocks.query.data = { run: { _tag: "Failed" } }; + screen.render(1, "environment", false); + expect(mocks.query.refresh).toHaveBeenCalledTimes(1); + screen.render(); + expect(mocks.query.refresh).toHaveBeenCalledTimes(2); + act(() => vi.advanceTimersByTime(10_000)); + expect(mocks.query.refresh).toHaveBeenCalledTimes(2); + expect(vi.getTimerCount()).toBe(0); +}); + +it("leaves generation running when hidden and accepts its completion without a refresh", async () => { + vi.useFakeTimers(); + using _timers = { [Symbol.dispose]: () => vi.useRealTimers() }; + using screen = setup(); + const generated = Promise.withResolvers>>(); + mocks.generate.mockReturnValue(generated.promise); + screen.render(); + let writing: Promise; + act(() => { + writing = screen.current.write(); + }); + expect(screen.current.running).toBe(true); + screen.render(1, "environment", false); + const beforeHidden = mocks.query.refresh.mock.calls.length; + act(() => vi.advanceTimersByTime(60_000)); + expect(mocks.query.refresh).toHaveBeenCalledTimes(beforeHidden); + expect(mocks.generate).toHaveBeenCalledTimes(1); + expect(screen.current.running).toBe(true); + await act(async () => { + generated.resolve(AsyncResult.success(undefined)); + await writing; + }); + expect(screen.current.running).toBe(false); + expect(mocks.query.refresh).toHaveBeenCalledTimes(beforeHidden); + screen.render(); + expect(mocks.query.refresh).toHaveBeenCalledTimes(beforeHidden + 1); + expect(vi.getTimerCount()).toBe(0); +}); diff --git a/apps/mobile/src/features/pull-requests/guide/useReviewGuide.ts b/apps/mobile/src/features/pull-requests/guide/useReviewGuide.ts index 5dad083e76108..bbdb6caee00c3 100644 --- a/apps/mobile/src/features/pull-requests/guide/useReviewGuide.ts +++ b/apps/mobile/src/features/pull-requests/guide/useReviewGuide.ts @@ -17,7 +17,7 @@ export function useReviewGuidesEnabled(params: PullRequestParams) { return settings?.reviewGuidesEnabled === true; } -export function useReviewGuide(params: PullRequestParams) { +export function useReviewGuide(params: PullRequestParams, visible = true) { const target = pullRequestTarget(params); const enabled = useReviewGuidesEnabled(params); const query = useEnvironmentQuery(enabled ? composerPullRequests.reviewGuide(target) : null); @@ -35,7 +35,7 @@ export function useReviewGuide(params: PullRequestParams) { const running = starting || query.data?.run?._tag === "Running"; const refresh = query.refresh; useFocusEffect(() => { - if (!enabled) return; + if (!enabled || !visible) return; refresh(); if (!running) return; const timer = setInterval(refresh, 5_000); diff --git a/apps/mobile/vite.config.ts b/apps/mobile/vite.config.ts index c341dec4e2b9a..2df90af2ee6fe 100644 --- a/apps/mobile/vite.config.ts +++ b/apps/mobile/vite.config.ts @@ -13,6 +13,7 @@ const reactTests = [ "src/features/pull-requests/usePullRequestDiff.test.tsx", "src/components/ui/Dialog.android.test.tsx", "src/features/pull-requests/guide/useReviewGuide.test.tsx", + "src/features/pull-requests/guide/PullRequestGuideVisibility.test.tsx", "src/features/settings/SettingsServerControlsRouteScreen.test.tsx", "src/features/settings/components/BranchNamingSettings.test.tsx", "src/features/threads/ThreadMarkdownImage.test.tsx", From fdcc9a79d5242bda3ad3d2017a3f3659ba5b5b7d Mon Sep 17 00:00:00 2001 From: maria-rcks Date: Tue, 27 Oct 2026 02:47:58 +0000 Subject: [PATCH 2/2] refactor(mobile): keep guide visibility regressions in the hook suite --- .../guide/PullRequestGuideVisibility.test.tsx | 446 ------------------ apps/mobile/vite.config.ts | 1 - 2 files changed, 447 deletions(-) delete mode 100644 apps/mobile/src/features/pull-requests/guide/PullRequestGuideVisibility.test.tsx diff --git a/apps/mobile/src/features/pull-requests/guide/PullRequestGuideVisibility.test.tsx b/apps/mobile/src/features/pull-requests/guide/PullRequestGuideVisibility.test.tsx deleted file mode 100644 index c4d20e0786db6..0000000000000 --- a/apps/mobile/src/features/pull-requests/guide/PullRequestGuideVisibility.test.tsx +++ /dev/null @@ -1,446 +0,0 @@ -import { - ProviderInstanceId, - type PullRequestListEntry, - type PullRequestReviewGuideResult, -} from "@t3tools/contracts"; -import { RegistryContext } from "@effect/atom-react"; -import { - NavigationContext, - type NavigationProp, - type ParamListBase, -} from "@react-navigation/native"; -import { AsyncResult, Atom, AtomRegistry } from "effect/reactivity"; -import { act, type ReactNode } from "react"; -import { createRoot } from "react-dom/client"; -import { expect, it, vi } from "vite-plus/test"; - -const mocks = vi.hoisted(() => ({ - guide: null as unknown, - settings: null as unknown, - permission: null as unknown, - empty: null as unknown, - refresh: vi.fn(), -})); - -vi.mock("@effect/atom-react", async (importOriginal) => { - const actual = await importOriginal(); - const { useMemo } = await import("react"); - return { - ...actual, - useAtomRefresh: (atom: Parameters[0]) => { - const refresh = actual.useAtomRefresh(atom); - return useMemo( - () => () => { - if (atom === mocks.guide) mocks.refresh(); - refresh(); - }, - [atom, refresh], - ); - }, - }; -}); -vi.mock("@react-navigation/native", async () => { - const { createRequire } = await import("node:module"); - const { realpathSync } = await import("node:fs"); - const require = createRequire(import.meta.url); - const nativeRequire = createRequire( - realpathSync(require.resolve("@react-navigation/native/package.json")), - ); - const core: typeof import("@react-navigation/native") = nativeRequire("@react-navigation/core"); - return { - NavigationContext: core.NavigationContext, - useFocusEffect: core.useFocusEffect, - useNavigation: core.useNavigation, - StackActions: core.StackActions, - }; -}); -vi.mock("react-native", () => { - const View = ({ - children, - accessibilityElementsHidden, - }: { - children?: ReactNode; - accessibilityElementsHidden?: boolean; - }) => ; - return { - View, - ScrollView: View, - Pressable: View, - RefreshControl: () => null, - Platform: { OS: "ios" }, - StyleSheet: { absoluteFill: {} }, - }; -}); -vi.mock("react-native-reanimated", () => ({ - default: { View: ({ children }: { children?: ReactNode }) =>
{children}
}, - FadeIn: { delay: () => ({ duration: () => ({ reduceMotion: () => ({}) }) }) }, - ReduceMotion: { System: "system" }, -})); -vi.mock("@legendapp/list/reanimated", () => ({ - // Render retained rows across section changes; native list virtualization is parent-owned proof. - AnimatedLegendList: ({ - data, - renderItem, - ListHeaderComponent, - }: { - data: { key: string; content: ReactNode }[]; - renderItem: (row: { item: { key: string; content: ReactNode } }) => ReactNode; - ListHeaderComponent: ReactNode; - }) => ( -
- {ListHeaderComponent} - {data.map((item) => ( -
{renderItem({ item })}
- ))} -
- ), -})); -vi.mock("react-native-safe-area-context", () => ({ - useSafeAreaInsets: () => ({ top: 0, bottom: 0 }), -})); -vi.mock("expo-clipboard", () => ({})); -vi.mock("../pullRequestLink", () => ({ - copyPullRequestLink: () => {}, - sharePullRequestLink: () => {}, -})); -vi.mock("@pierre/diffs/utils/parsePatchFiles", () => ({ parsePatchFiles: () => [] })); -vi.mock("@t3tools/client-runtime/source-control-clients", () => ({ - sourceControlClients: { get: () => ({ changeRequest: { singular: "pull request" } }) }, - capitalizeTerm: (term: string) => term, -})); -vi.mock("../../../state/pull-requests", () => ({ - composerPullRequests: { - reviewGuide: () => mocks.guide, - detail: () => mocks.empty, - activity: () => mocks.empty, - generateReviewGuide: { permissionAtom: () => mocks.permission }, - }, -})); -vi.mock("../../../state/server", () => ({ - serverEnvironment: { settingsValueAtom: () => mocks.settings }, -})); -vi.mock("../../../state/use-atom-command", () => ({ useAtomCommand: () => vi.fn() })); -vi.mock("../../../state/use-pull-to-refresh", () => ({ - usePullToRefresh: () => ({ refreshing: false }), -})); -vi.mock("../../../components/ui/Toolbar", () => { - const Empty = () => null; - return { Toolbar: Object.assign(Empty, { Menu: Empty, MenuAction: Empty, Label: Empty }) }; -}); -vi.mock("../../../components/ui/AppText", () => ({ - AppText: ({ children }: { children?: ReactNode }) => {children}, -})); -vi.mock("../../../components/ui/Button", () => ({ - Button: ({ - label, - disabled, - onPress, - }: { - label: string; - disabled?: boolean; - onPress: () => void; - }) => ( - - ), -})); -vi.mock("../../../components/ui/SegmentedControl", () => ({ - SegmentedControl: ({ - options, - onSelect, - }: { - options: { value: string; label: string }[]; - onSelect: (value: string) => void; - }) => ( - - ), -})); -vi.mock("../../../components/ui/ScreenScrollView", () => ({ - ScreenScrollView: ({ children }: { children?: ReactNode }) =>
{children}
, -})); -vi.mock("../../../native/StackHeader", () => ({ NativeStackScreenOptions: () => null })); -vi.mock("../../../native/NativeScrollViewMarker", () => ({ - NativeScrollViewMarker: ({ children }: { children?: ReactNode }) =>
{children}
, -})); -vi.mock("../../../components/ui/useAndroidControlSizing", () => ({ - useAndroidControlSizing: () => ({ buttonSize: 32 }), -})); -vi.mock("../../layout/AdaptiveWorkspaceLayout", () => ({ - useAdaptiveWorkspaceLayout: () => ({ layout: { usesSplitView: false } }), -})); -vi.mock("../../layout/useWorkspaceSidebarHeaderItem", () => ({ - useWorkspaceSidebarHeaderItem: () => null, -})); -vi.mock("../../workspace/useAndroidWorkspaceNavigation", () => ({ - useAndroidWorkspaceBack: () => vi.fn(), -})); -vi.mock("../usePullRequestConnected", () => ({ usePullRequestConnected: () => true })); -vi.mock("../usePullRequestLiveChecks", () => ({ usePullRequestLiveChecks: () => null })); -vi.mock("../usePullRequestEnvironmentFallback", () => ({ - usePullRequestEnvironmentFallback: () => null, -})); -vi.mock("../usePullRequestPermissions", () => ({ - usePullRequestPermissions: () => ({ verdicts: [] }), -})); -vi.mock("../usePullRequestAction", () => ({ usePullRequestAction: () => ({}) })); -vi.mock("../usePullRequestStack", () => ({ usePullRequestStack: () => ({}) })); -vi.mock("../PullRequestFiles", () => ({ usePullRequestFilesSection: () => ({ rows: [] }) })); -vi.mock("../PullRequestTimeline", () => ({ pullRequestTimelineRows: () => [] })); -vi.mock("../PullRequestMarkdown", () => ({ - PullRequestMarkdownRepository: ({ children }: { children?: ReactNode }) => <>{children}, - PullRequestMarkdown: () => null, -})); -vi.mock("../PullRequestMedia", async () => ({ - PullRequestMediaVisibleContext: (await import("react")).createContext(true), -})); -vi.mock("../usePullRequestDiff", () => ({ - pullRequestTarget: (params: unknown) => params, - usePullRequestDiff: () => ({ patch: "", parsed: { kind: "files", files: [] } }), -})); -vi.mock("../../settings/appearance/AppearancePreferencesProvider", () => ({ - useAppearancePreferences: () => ({ appearance: {} }), -})); -vi.mock("./GuideOutline", () => ({ - GuideOutline: ({ guide }: { guide: { headSha: string } }) =>

Cached guide {guide.headSha}

, -})); -vi.mock("../../../components/ui/AppActivityIndicator", () => ({ - AppActivityIndicator: () => null, -})); -vi.mock("../../../components/ui/Chip", () => ({ Chip: () => null })); -vi.mock("../../../components/ui/Menu", () => ({ Menu: () => null })); -vi.mock("../../../components/ui/IconButton", () => ({ IconButton: () => null })); -vi.mock("../../../components/ui/AndroidScreenHeader", () => ({ AndroidScreenHeader: () => null })); -vi.mock("../../../components/ui/EmptyState", () => ({ EmptyState: () => null })); -vi.mock("../../../components/ui/AppSymbol", () => ({ SymbolView: () => null })); -vi.mock("../../layout/workspace-sidebar-toolbar", () => ({ - AndroidWorkspaceSidebarButton: () => null, -})); -vi.mock("../PullRequestActivityFilters", () => ({ PullRequestActivityFilters: () => null })); -vi.mock("../PullRequestOverviewLoading", () => ({ PullRequestOverviewLoading: () => null })); -vi.mock("../PullRequestPresentation", () => ({ - PullRequestAvatar: () => null, - PullRequestDiffStats: () => null, - PullRequestReadStatus: () => null, -})); -vi.mock("../PullRequestControls", () => ({ - PullRequestActionToolbar: () => null, - PullRequestCardGroup: () => null, - PullRequestCardRow: () => null, -})); -vi.mock("../PullRequestStatusRows", () => ({ - PullRequestStatusMenu: () => null, - PullRequestChecksRow: () => null, - PullRequestBranchRow: () => null, - PullRequestReviewersRow: () => null, -})); -vi.mock("../PullRequestMergeControls", () => ({ PullRequestMergeControls: () => null })); -vi.mock("../PullRequestStackCard", () => ({ PullRequestStackCard: () => null })); -vi.mock("../PullRequestReactions", () => ({ PullRequestReactions: () => null })); -vi.mock("../PullRequestDescription", () => ({ PullRequestDescription: () => null })); -vi.mock("./ReviewGuideExcerpt", () => ({ ReviewGuideExcerpt: () => null })); -vi.mock("../usePullRequestReviewDraft", () => ({ usePullRequestReviewDraft: () => null })); -vi.mock("../usePullRequestFilesViewed", () => ({ usePullRequestFilesViewed: () => null })); -vi.mock("../usePullRequestFileFilter", () => ({ usePullRequestFileFilter: () => null })); -vi.mock("../usePullRequestDiffHighlightFiles", () => ({ - usePullRequestDiffHighlightFiles: () => null, -})); -vi.mock("../../settings/appearance/useAppearanceCodeSurface", () => ({ - useAppearanceCodeSurface: () => null, -})); -vi.mock("../../review/useNativeReviewDiffBridge", () => ({ - useNativeReviewDiffBridge: () => null, -})); -vi.mock("../../diffs/nativeReviewDiffSurface", () => ({ resolveNativeReviewDiffView: () => null })); -vi.mock("../../review/nativeReviewDiffAdapter", () => ({ - getCachedNativeReviewDiffData: () => null, -})); -vi.mock("../pullRequestAnimations", () => ({ - PR_ROW_LAYOUT: {}, - PR_ROW_ENTER: {}, - PR_ROW_EXIT: {}, - PR_POSITION_LAYOUT: {}, -})); - -import { PullRequestRouteScreen } from "../PullRequestRouteScreen"; -import { PullRequestGuideStopScreen } from "./PullRequestGuideScreen"; - -function setup() { - (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; - mocks.refresh = vi.fn(); - const initial: PullRequestReviewGuideResult = { - currentHeadSha: "cached-head", - guide: { - headSha: "cached-head", - generatedAt: "2026-10-26T00:00:00Z", - modelSelection: { instanceId: ProviderInstanceId.make("codex"), model: "gpt-6.1-sol" }, - stops: [], - }, - run: { _tag: "Running", headSha: "cached-head", startedAt: "2026-10-26T00:00:00Z" }, - }; - const source = Atom.make(AsyncResult.success(initial)); - let acquisitions = 0; - let releases = 0; - const guide = Atom.make((get) => { - acquisitions++; - get.addFinalizer(() => { - releases++; - }); - return get(source); - }).pipe(Atom.setIdleTTL("0 millis")); - mocks.guide = guide; - mocks.empty = Atom.make(AsyncResult.success(null)); - mocks.settings = Atom.make({ reviewGuidesEnabled: true }); - mocks.permission = Atom.make(true); - const registry = AtomRegistry.make(); - const container = document.createElement("div"); - const root = createRoot(container); - const listeners = { focus: new Set<() => void>(), blur: new Set<() => void>() }; - let focused = true; - const navigation = { - isFocused: () => focused, - addListener: (event: keyof typeof listeners, listener: () => void) => { - listeners[event].add(listener); - return () => listeners[event].delete(listener); - }, - } as unknown as NavigationProp; - const params = { - environmentId: "environment", - projectId: "project", - repository: "repo", - number: 1, - initialSummary: { - title: "Test PR", - state: "open", - isDraft: false, - provider: "github", - updatedAt: "2026-10-26T00:00:00Z", - author: null, - baseBranch: "main", - headBranch: "change", - } as PullRequestListEntry, - }; - return { - container, - render(stop = false) { - act(() => - root.render( - - - - {stop ? ( - - ) : null} - - , - ), - ); - }, - select(label: string) { - const button = [...container.querySelectorAll("button")].find( - (item) => item.textContent === label, - ); - if (!button) throw new Error(`Missing section ${label}`); - act(() => button.click()); - }, - complete() { - act(() => registry.set(source, AsyncResult.success({ ...initial, run: null }))); - }, - focus(next: boolean) { - focused = next; - act(() => { - for (const listener of listeners[next ? "focus" : "blur"]) listener(); - }); - }, - get acquisitions() { - return acquisitions; - }, - get releases() { - return releases; - }, - [Symbol.dispose]() { - act(() => root.unmount()); - registry.dispose(); - expect(releases).toBe(acquisitions); - expect(listeners.focus.size).toBe(0); - expect(listeners.blur.size).toBe(0); - }, - }; -} - -it("retains the production Guide query and cached outline while hidden, then refreshes once on reveal", () => { - vi.useFakeTimers(); - using _timers = { [Symbol.dispose]: () => vi.useRealTimers() }; - { - using screen = setup(); - screen.render(); - screen.select("Guide"); - const outline = screen.container.querySelector("p"); - expect(outline?.textContent).toBe("Cached guide cached-head"); - expect(screen.container.textContent).toContain("Rewriting guide"); - screen.select("Overview"); - const beforeHidden = mocks.refresh.mock.calls.length; - const acquisitions = screen.acquisitions; - const releases = screen.releases; - act(() => vi.advanceTimersByTime(60_000)); - expect({ - manualRefreshes: mocks.refresh.mock.calls.length - beforeHidden, - queryAcquisitions: screen.acquisitions - acquisitions, - }).toEqual({ manualRefreshes: 0, queryAcquisitions: 0 }); - expect(screen.releases).toBe(releases); - expect(screen.container.querySelector("p")).toBe(outline); - - screen.complete(); - expect(screen.container.textContent).not.toContain("Rewriting guide"); - expect(mocks.refresh).toHaveBeenCalledTimes(beforeHidden); - expect(screen.acquisitions).toBeGreaterThan(acquisitions); - screen.select("Guide"); - expect(mocks.refresh).toHaveBeenCalledTimes(beforeHidden + 1); - expect(screen.container.querySelector("p")).toBe(outline); - act(() => vi.advanceTimersByTime(10_000)); - expect(mocks.refresh).toHaveBeenCalledTimes(beforeHidden + 1); - } - // AtomRegistry defers node removal to its zero-delay dispatcher. - act(() => vi.advanceTimersByTime(0)); - expect(vi.getTimerCount()).toBe(0); -}); - -it("keeps the standalone GuideStop reader refreshing while the retained pane is hidden", () => { - vi.useFakeTimers(); - using _timers = { [Symbol.dispose]: () => vi.useRealTimers() }; - { - using screen = setup(); - screen.render(true); - const initial = mocks.refresh.mock.calls.length; - act(() => vi.advanceTimersByTime(5_000)); - expect(mocks.refresh).toHaveBeenCalledTimes(initial + 1); - screen.focus(false); - act(() => vi.advanceTimersByTime(5_000)); - expect(mocks.refresh).toHaveBeenCalledTimes(initial + 1); - screen.focus(true); - expect(mocks.refresh).toHaveBeenCalledTimes(initial + 2); - screen.render(false); - const afterStop = mocks.refresh.mock.calls.length; - act(() => vi.advanceTimersByTime(60_000)); - expect(mocks.refresh).toHaveBeenCalledTimes(afterStop); - expect(vi.getTimerCount()).toBe(0); - } - // AtomRegistry defers node removal to its zero-delay dispatcher. - act(() => vi.advanceTimersByTime(0)); - expect(vi.getTimerCount()).toBe(0); -}); diff --git a/apps/mobile/vite.config.ts b/apps/mobile/vite.config.ts index 2df90af2ee6fe..c341dec4e2b9a 100644 --- a/apps/mobile/vite.config.ts +++ b/apps/mobile/vite.config.ts @@ -13,7 +13,6 @@ const reactTests = [ "src/features/pull-requests/usePullRequestDiff.test.tsx", "src/components/ui/Dialog.android.test.tsx", "src/features/pull-requests/guide/useReviewGuide.test.tsx", - "src/features/pull-requests/guide/PullRequestGuideVisibility.test.tsx", "src/features/settings/SettingsServerControlsRouteScreen.test.tsx", "src/features/settings/components/BranchNamingSettings.test.tsx", "src/features/threads/ThreadMarkdownImage.test.tsx",