From db7cca75e7219a1f90ec593eff013944f2c0d0a2 Mon Sep 17 00:00:00 2001 From: SAPHID Date: Mon, 12 Oct 2026 23:24:02 +0000 Subject: [PATCH 1/2] perf(mobile): index viewed files before rendering pull requests Co-Authored-By: GPT-6.1 Sol --- .../usePullRequestFilesViewed.test.tsx | 121 ++++++++++++++++++ .../usePullRequestFilesViewed.ts | 17 ++- 2 files changed, 129 insertions(+), 9 deletions(-) create mode 100644 apps/mobile/src/features/pull-requests/usePullRequestFilesViewed.test.tsx diff --git a/apps/mobile/src/features/pull-requests/usePullRequestFilesViewed.test.tsx b/apps/mobile/src/features/pull-requests/usePullRequestFilesViewed.test.tsx new file mode 100644 index 0000000000000..eb69f277742a2 --- /dev/null +++ b/apps/mobile/src/features/pull-requests/usePullRequestFilesViewed.test.tsx @@ -0,0 +1,121 @@ +// @vitest-environment jsdom +import type { PullRequestDetail, PullRequestFilesViewedResult } from "@t3tools/contracts"; +import { act, useEffect } from "react"; +import { createRoot, type Root } from "react-dom/client"; +import { afterEach, beforeEach, expect, it, vi } from "vite-plus/test"; + +const host = vi.hoisted(() => ({ + data: null as PullRequestFilesViewedResult | null, + write: vi.fn(), + refresh: vi.fn(), +})); +vi.mock("@effect/atom-react", () => ({ + useAtomValue: () => true, + useAtomRefresh: () => host.refresh, +})); +vi.mock("../../state/pull-requests", () => ({ + composerPullRequests: { + filesViewed: () => null, + setFilesViewed: { permissionAtom: () => null }, + }, +})); +vi.mock("../../state/query", () => ({ + useEnvironmentQuery: () => ({ data: host.data, error: null }), +})); +vi.mock("../../state/use-atom-command", () => ({ useAtomCommand: () => host.write })); +vi.mock("./usePullRequestConnected", () => ({ usePullRequestConnected: () => true })); +vi.mock("./usePullRequestDiff", () => ({ + pullRequestTarget: (params: unknown) => ({ environmentId: "env", input: params }), +})); + +import { usePullRequestFilesViewed } from "./usePullRequestFilesViewed"; + +const params = { environmentId: "env", projectId: "project", repository: "owner/repo", number: 1 }; +const pr = { capabilities: { viewedFiles: true }, headSha: "head" } as unknown as PullRequestDetail; +let root: Root; +let view: ReturnType; +function Surface() { + const current = usePullRequestFilesViewed(params, pr); + useEffect(() => { + view = current; + }); + return null; +} +function render() { + act(() => root.render()); +} +beforeEach(() => { + vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); + host.data = null; + host.write.mockReset().mockResolvedValue({ _tag: "Success" }); + host.refresh.mockReset(); + root = createRoot(document.createElement("div")); +}); +afterEach(() => { + act(() => root.unmount()); + vi.unstubAllGlobals(); +}); + +it("resolves server states, optimistic marks, repeated toggles, and refreshed answers", async () => { + render(); + expect(view.isViewed("missing")).toBe(false); + host.data = { + files: [ + { path: "a", state: "viewed" }, + { path: "b", state: "unviewed" }, + { path: "c", state: "dismissed" }, + ], + truncated: false, + }; + render(); + expect(["a", "b", "c", "missing"].map(view.isViewed)).toEqual([true, false, false, false]); + await act(async () => { + await view.toggle("a"); + await view.toggle("b"); + await view.toggle("c"); + }); + expect(["a", "b", "c"].map(view.isViewed)).toEqual([false, true, true]); + await act(async () => { + await view.toggle("b"); + }); + expect(view.isViewed("b")).toBe(false); + expect(host.write.mock.calls.map(([request]) => request.input.files)).toEqual([ + [{ path: "a", viewed: false }], + [{ path: "b", viewed: true }], + [{ path: "c", viewed: true }], + [{ path: "b", viewed: false }], + ]); + host.data = { + files: [ + { path: "c", state: "dismissed" }, + { path: "b", state: "viewed" }, + ], + truncated: false, + }; + render(); + expect(["a", "b", "c"].map(view.isViewed)).toEqual([false, true, false]); +}); + +it("reads server paths once per answer rather than once per rendered file", async () => { + let pathReads = 0; + host.data = { + files: Array.from({ length: 500 }, (_, index) => ({ + get path() { + pathReads += 1; + return `server-${index}`; + }, + state: "viewed" as const, + })), + truncated: false, + }; + render(); + for (let pass = 0; pass < 2; pass += 1) { + for (let index = 0; index < 3000; index += 1) + expect(view.isViewed(`file-${index}`)).toBe(false); + } + render(); + await act(async () => { + await view.toggle("file-0"); + }); + expect(pathReads).toBeLessThanOrEqual(500); +}); diff --git a/apps/mobile/src/features/pull-requests/usePullRequestFilesViewed.ts b/apps/mobile/src/features/pull-requests/usePullRequestFilesViewed.ts index cf882ac0daee2..accfbf4fa55aa 100644 --- a/apps/mobile/src/features/pull-requests/usePullRequestFilesViewed.ts +++ b/apps/mobile/src/features/pull-requests/usePullRequestFilesViewed.ts @@ -2,7 +2,7 @@ import { usePullRequestConnected } from "./usePullRequestConnected"; import { useAtomRefresh, useAtomValue } from "@effect/atom-react"; import { squashAtomCommandFailure } from "@t3tools/client-runtime/state/runtime"; import type { PullRequestDetail } from "@t3tools/contracts"; -import { useCallback, useEffect, useRef, useState } from "react"; +import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import { composerPullRequests } from "../../state/pull-requests"; import { useEnvironmentQuery } from "../../state/query"; import { useAtomCommand } from "../../state/use-atom-command"; @@ -19,6 +19,11 @@ export function usePullRequestFilesViewed(params: PullRequestParams, pr: PullReq const connected = usePullRequestConnected(params.environmentId); const allowed = connected && permission; const query = useEnvironmentQuery(supported ? composerPullRequests.filesViewed(target) : null); + const viewedPaths = useMemo( + () => + new Set(query.data?.files.filter((file) => file.state === "viewed").map((file) => file.path)), + [query.data], + ); const refresh = useAtomRefresh(composerPullRequests.filesViewed(target)); const write = useAtomCommand(composerPullRequests.setFilesViewed, { reportFailure: false }); const [state, setState] = useState<{ @@ -74,16 +79,10 @@ export function usePullRequestFilesViewed(params: PullRequestParams, pr: PullReq setOverlay(next); } }, [query.data, setOverlay]); - const isViewed = (path: string) => - overlay[path] ?? - query.data?.files.some((file) => file.path === path && file.state === "viewed") ?? - false; + const isViewed = (path: string) => overlay[path] ?? viewedPaths.has(path); const toggle = async (path: string) => { if (!supported || !allowed || !query.data) return; - const viewed = !( - pressed.current[path] ?? - query.data.files.some((file) => file.path === path && file.state === "viewed") - ); + const viewed = !(pressed.current[path] ?? viewedPaths.has(path)); const version = (versions.current.get(path) ?? 0) + 1; versions.current.set(path, version); awaitingRead.current.delete(path); From 519abd2327a0b3c959fedb9a122202a6dcf2bfb7 Mon Sep 17 00:00:00 2001 From: SAPHID Date: Wed, 14 Oct 2026 17:15:52 +0000 Subject: [PATCH 2/2] test(mobile): drop the read-counting viewed files test The Set lookup is a small cleanup; the removed test counted getter reads, which mirrored the implementation instead of checking behavior. Co-Authored-By: Claude Opus 5.5 --- .../usePullRequestFilesViewed.test.tsx | 121 ------------------ 1 file changed, 121 deletions(-) delete mode 100644 apps/mobile/src/features/pull-requests/usePullRequestFilesViewed.test.tsx diff --git a/apps/mobile/src/features/pull-requests/usePullRequestFilesViewed.test.tsx b/apps/mobile/src/features/pull-requests/usePullRequestFilesViewed.test.tsx deleted file mode 100644 index eb69f277742a2..0000000000000 --- a/apps/mobile/src/features/pull-requests/usePullRequestFilesViewed.test.tsx +++ /dev/null @@ -1,121 +0,0 @@ -// @vitest-environment jsdom -import type { PullRequestDetail, PullRequestFilesViewedResult } from "@t3tools/contracts"; -import { act, useEffect } from "react"; -import { createRoot, type Root } from "react-dom/client"; -import { afterEach, beforeEach, expect, it, vi } from "vite-plus/test"; - -const host = vi.hoisted(() => ({ - data: null as PullRequestFilesViewedResult | null, - write: vi.fn(), - refresh: vi.fn(), -})); -vi.mock("@effect/atom-react", () => ({ - useAtomValue: () => true, - useAtomRefresh: () => host.refresh, -})); -vi.mock("../../state/pull-requests", () => ({ - composerPullRequests: { - filesViewed: () => null, - setFilesViewed: { permissionAtom: () => null }, - }, -})); -vi.mock("../../state/query", () => ({ - useEnvironmentQuery: () => ({ data: host.data, error: null }), -})); -vi.mock("../../state/use-atom-command", () => ({ useAtomCommand: () => host.write })); -vi.mock("./usePullRequestConnected", () => ({ usePullRequestConnected: () => true })); -vi.mock("./usePullRequestDiff", () => ({ - pullRequestTarget: (params: unknown) => ({ environmentId: "env", input: params }), -})); - -import { usePullRequestFilesViewed } from "./usePullRequestFilesViewed"; - -const params = { environmentId: "env", projectId: "project", repository: "owner/repo", number: 1 }; -const pr = { capabilities: { viewedFiles: true }, headSha: "head" } as unknown as PullRequestDetail; -let root: Root; -let view: ReturnType; -function Surface() { - const current = usePullRequestFilesViewed(params, pr); - useEffect(() => { - view = current; - }); - return null; -} -function render() { - act(() => root.render()); -} -beforeEach(() => { - vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); - host.data = null; - host.write.mockReset().mockResolvedValue({ _tag: "Success" }); - host.refresh.mockReset(); - root = createRoot(document.createElement("div")); -}); -afterEach(() => { - act(() => root.unmount()); - vi.unstubAllGlobals(); -}); - -it("resolves server states, optimistic marks, repeated toggles, and refreshed answers", async () => { - render(); - expect(view.isViewed("missing")).toBe(false); - host.data = { - files: [ - { path: "a", state: "viewed" }, - { path: "b", state: "unviewed" }, - { path: "c", state: "dismissed" }, - ], - truncated: false, - }; - render(); - expect(["a", "b", "c", "missing"].map(view.isViewed)).toEqual([true, false, false, false]); - await act(async () => { - await view.toggle("a"); - await view.toggle("b"); - await view.toggle("c"); - }); - expect(["a", "b", "c"].map(view.isViewed)).toEqual([false, true, true]); - await act(async () => { - await view.toggle("b"); - }); - expect(view.isViewed("b")).toBe(false); - expect(host.write.mock.calls.map(([request]) => request.input.files)).toEqual([ - [{ path: "a", viewed: false }], - [{ path: "b", viewed: true }], - [{ path: "c", viewed: true }], - [{ path: "b", viewed: false }], - ]); - host.data = { - files: [ - { path: "c", state: "dismissed" }, - { path: "b", state: "viewed" }, - ], - truncated: false, - }; - render(); - expect(["a", "b", "c"].map(view.isViewed)).toEqual([false, true, false]); -}); - -it("reads server paths once per answer rather than once per rendered file", async () => { - let pathReads = 0; - host.data = { - files: Array.from({ length: 500 }, (_, index) => ({ - get path() { - pathReads += 1; - return `server-${index}`; - }, - state: "viewed" as const, - })), - truncated: false, - }; - render(); - for (let pass = 0; pass < 2; pass += 1) { - for (let index = 0; index < 3000; index += 1) - expect(view.isViewed(`file-${index}`)).toBe(false); - } - render(); - await act(async () => { - await view.toggle("file-0"); - }); - expect(pathReads).toBeLessThanOrEqual(500); -});