diff --git a/apps/mobile/src/components/pr-review/diff/pr-diff-file-list-render.test.tsx b/apps/mobile/src/components/pr-review/diff/pr-diff-file-list-render.test.tsx new file mode 100644 index 00000000000..f7e7f5cf17f --- /dev/null +++ b/apps/mobile/src/components/pr-review/diff/pr-diff-file-list-render.test.tsx @@ -0,0 +1,267 @@ +// `useDiffRenderItem` binds one identity-stable `onTap` per rendered diff line, +// so `DiffLine`'s memo comparator — which compares `onTap` by reference +// (`diff-line.tsx`) — can hit. A fresh closure per line per render defeated it, +// so tapping one line re-rendered every mounted diff row instead of only the +// rows whose `isSelected` changed. These tests pin the three behaviours that +// make the memo hit: +// (a) the element's `onTap` prop keeps the same reference across a re-render +// that only changes `selection`; +// (b) the cached callback reads the current item (and the current `onLineTap`) +// through refs, so it never goes stale when the item is rebuilt; +// (c) a non-selectable line still gets `onTap === undefined`. + +import { createElement, type ReactElement } from 'react'; +import { act, TestRenderer } from '@/test/renderer'; +import { describe, expect, it, vi } from 'vitest'; + +import { + type LineTapArgs, + useDiffRenderItem, +} from '@/components/pr-review/diff/pr-diff-file-list-render'; +import { type ListItem } from '@/lib/pr-review/diff/pr-diff-list-items'; +import { type ParsedDiffLine, type ParsedPatch } from '@/lib/pr-review/diff/parse-patch'; + +// The heavy children are string hosts: this suite exercises only the props +// `useDiffRenderItem` binds, not how the row itself renders. +vi.mock('react-native', () => ({ + View: 'View', + Pressable: 'Pressable', + Text: 'Text', +})); +vi.mock('@/components/pr-review/diff/diff-line', () => ({ DiffLine: 'DiffLine' })); +vi.mock('@/components/pr-review/diff/pr-diff-rows', () => ({ + ExpandSeparatorRow: 'ExpandSeparatorRow', + FileHeaderRow: 'FileHeaderRow', + HunkHeaderRow: 'HunkHeaderRow', + PaginationRow: 'PaginationRow', + PatchMissingRow: 'PatchMissingRow', + TruncationBannerRow: 'TruncationBannerRow', +})); +vi.mock('@/components/pr-review/diff/pr-diff-side-by-side-row', () => ({ + HunkSideBySideHeader: 'HunkSideBySideHeader', + SideBySideRow: 'SideBySideRow', +})); + +type DiffLineItem = Extract; + +type SelectionView = { + filePath: string; + side: 'LEFT' | 'RIGHT'; + startLine: number; + line: number; +} | null; + +type HarnessProps = { + item: DiffLineItem; + onLineTap: (args: LineTapArgs) => void; + selection: SelectionView; + /** When false the row is unmounted while the hook stays mounted. */ + show?: boolean; +}; + +// A minimal host for the hook: mount it, then `renderer.update` with a new +// `selection` / item to force the re-render the memo has to survive. +function Harness({ item, onLineTap, selection, show = true }: Readonly) { + const renderItem = useDiffRenderItem({ + viewed: { isViewed: () => false, toggle: vi.fn() }, + onRetryPage: () => undefined, + onFetchAll: () => undefined, + handleLoadContext: () => undefined, + setExpanded: () => undefined, + onLineTap, + selection, + }); + return <>{show ? renderItem({ item }) : null}; +} + +// One builder for the whole line, so a rebuilt item keeps the same `key` (the +// map binds by key) but changes every argument the tap carries. +function buildItem(text: string, newLine: number, selectable?: boolean): DiffLineItem { + const line: ParsedDiffLine = { type: 'add', newLine, text, noNewlineAtEndOfFile: false }; + const parsed: ParsedPatch = { + isRename: false, + hunks: [ + { + header: '@@ -1,1 +1,2 @@', + oldStart: 1, + oldLines: 1, + newStart: 1, + newLines: 2, + lines: [line], + }, + ], + }; + return { + kind: 'diff-line', + key: 'line:src/a.ts:0:0', + lineKey: 'src/a.ts:0:0', + filePath: 'src/a.ts', + hunkIndex: 0, + lineIndex: 0, + parsed, + line, + language: 'typescript', + lineKeyId: 'line:src/a.ts:0:0', + ...(selectable === undefined ? {} : { selectable }), + }; +} + +function mount(node: ReactElement): TestRenderer.ReactTestRenderer { + const ref: { current: TestRenderer.ReactTestRenderer | undefined } = { current: undefined }; + act(() => { + ref.current = TestRenderer.create(node); + }); + if (!ref.current) { + throw new Error('renderer was not created'); + } + return ref.current; +} + +function diffLineProps(renderer: TestRenderer.ReactTestRenderer): Record { + return renderer.root.find(node => String(node.type) === 'DiffLine').props; +} + +describe('useDiffRenderItem per-line onTap identity', () => { + it('keeps the same onTap reference across a selection-only re-render', () => { + const onLineTap = vi.fn<(args: LineTapArgs) => void>(); + const item = buildItem('beta', 2); + const renderer = mount(createElement(Harness, { item, onLineTap, selection: null })); + const before = diffLineProps(renderer).onTap; + expect(typeof before).toBe('function'); + + act(() => { + renderer.update( + createElement(Harness, { + item, + onLineTap, + selection: { filePath: 'src/a.ts', side: 'RIGHT', startLine: 2, line: 2 }, + }) + ); + }); + + expect(diffLineProps(renderer).onTap).toBe(before); + }); + + it('invokes onLineTap with the current item after the item is rebuilt', () => { + const onLineTap = vi.fn<(args: LineTapArgs) => void>(); + const renderer = mount( + createElement(Harness, { item: buildItem('beta', 2), onLineTap, selection: null }) + ); + const tap = diffLineProps(renderer).onTap as () => void; + + // Same key, new text / line number / parsed hunk, and a fresh `onLineTap`: + // a stale closure would still report the first render's values (or the + // first `onLineTap`). + const rebuilt = buildItem('beta changed', 7); + const nextOnLineTap = vi.fn<(args: LineTapArgs) => void>(); + act(() => { + renderer.update( + createElement(Harness, { item: rebuilt, onLineTap: nextOnLineTap, selection: null }) + ); + }); + + expect(diffLineProps(renderer).onTap).toBe(tap); + act(() => { + tap(); + }); + expect(nextOnLineTap).toHaveBeenCalledWith({ + filePath: 'src/a.ts', + hunkKey: 'src/a.ts:0', + side: 'RIGHT', + line: 7, + text: 'beta changed', + hunk: rebuilt.parsed.hunks[0], + }); + expect(onLineTap).not.toHaveBeenCalled(); + }); + + it('leaves a non-selectable line without a tap affordance', () => { + const onLineTap = vi.fn<(args: LineTapArgs) => void>(); + const renderer = mount( + createElement(Harness, { item: buildItem('gap', 9, false), onLineTap, selection: null }) + ); + + expect(diffLineProps(renderer).onTap).toBeUndefined(); + }); +}); + +// The cached callback + item are released when FlashList unmounts the row as it +// scrolls out of the render window. Before that release existed, the two maps +// kept one entry per line ever rendered — and each retained item holds its +// file's whole parsed patch, so a collapsed or refetched file's lines were +// never collected. +describe('useDiffRenderItem releases per-line state when a row unmounts', () => { + it('drops the cached tap callback so a remount builds a fresh one', () => { + const onLineTap = vi.fn<(args: LineTapArgs) => void>(); + const item = buildItem('beta', 2); + const renderer = mount(createElement(Harness, { item, onLineTap, selection: null })); + const before = diffLineProps(renderer).onTap; + expect(typeof before).toBe('function'); + + act(() => { + renderer.update(createElement(Harness, { item, onLineTap, selection: null, show: false })); + }); + expect(renderer.root.findAll(node => String(node.type) === 'DiffLine')).toHaveLength(0); + + act(() => { + renderer.update(createElement(Harness, { item, onLineTap, selection: null })); + }); + const after = diffLineProps(renderer).onTap; + expect(typeof after).toBe('function'); + // A retained entry would hand back the released callback. + expect(after).not.toBe(before); + }); + + it('drops both map entries for the unmounted line key', () => { + const deleteSpy = vi.spyOn(Map.prototype, 'delete'); + try { + const onLineTap = vi.fn<(args: LineTapArgs) => void>(); + const item = buildItem('beta', 2); + const renderer = mount(createElement(Harness, { item, onLineTap, selection: null })); + + act(() => { + renderer.update(createElement(Harness, { item, onLineTap, selection: null, show: false })); + }); + + // The item map and the callback map both key on the line key. + expect(deleteSpy.mock.calls.filter(([key]) => key === 'line:src/a.ts:0:0')).toHaveLength(2); + } finally { + deleteSpy.mockRestore(); + } + }); + + it('taps with the current item after the row remounts', () => { + const onLineTap = vi.fn<(args: LineTapArgs) => void>(); + const renderer = mount( + createElement(Harness, { item: buildItem('beta', 2), onLineTap, selection: null }) + ); + + act(() => { + renderer.update( + createElement(Harness, { + item: buildItem('beta', 2), + onLineTap, + selection: null, + show: false, + }) + ); + }); + const rebuilt = buildItem('beta again', 4); + act(() => { + renderer.update(createElement(Harness, { item: rebuilt, onLineTap, selection: null })); + }); + + const tap = diffLineProps(renderer).onTap as () => void; + act(() => { + tap(); + }); + expect(onLineTap).toHaveBeenCalledWith({ + filePath: 'src/a.ts', + hunkKey: 'src/a.ts:0', + side: 'RIGHT', + line: 4, + text: 'beta again', + hunk: rebuilt.parsed.hunks[0], + }); + }); +}); diff --git a/apps/mobile/src/components/pr-review/diff/pr-diff-file-list-render.tsx b/apps/mobile/src/components/pr-review/diff/pr-diff-file-list-render.tsx index b565be9feb8..1f484f634e3 100644 --- a/apps/mobile/src/components/pr-review/diff/pr-diff-file-list-render.tsx +++ b/apps/mobile/src/components/pr-review/diff/pr-diff-file-list-render.tsx @@ -3,7 +3,7 @@ // limit. Receives the full set of state needed to switch on item kind // and dispatch to the right row component. -import { useCallback } from 'react'; +import { useCallback, useEffect, useRef } from 'react'; import { DiffLine } from '@/components/pr-review/diff/diff-line'; import { @@ -61,6 +61,48 @@ export type LineTapArgs = { hunk: ParsedHunk; }; +/** The one item the per-line tap map is keyed on. */ +type DiffLineItem = Extract; + +// A thin per-row host whose only job is to release the line's cached tap +// callback + item when FlashList unmounts the row. `DiffLine`'s memo +// comparator stays on the inner element, so the identity stability the parent +// relies on is unchanged. +function DiffLineRow({ + item, + onTap, + isSelected, + register, + release, +}: Readonly<{ + item: DiffLineItem; + onTap: (() => void) | undefined; + isSelected: boolean; + register: (item: DiffLineItem) => void; + release: (key: string) => void; +}>) { + useEffect(() => { + register(item); + return () => { + release(item.key); + }; + // `item.key` is the row's identity and `register`/`release` are stable. + // The item itself is re-registered from `lineTapFor` on every render, so a + // rebuilt item must not re-run this effect: that would release the cached + // callback the memo comparator depends on. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [item.key, register, release]); + return ( + + ); +} + export function useDiffRenderItem({ viewed, onRetryPage, @@ -70,6 +112,70 @@ export function useDiffRenderItem({ onLineTap, selection, }: UseDiffRenderItemArgs) { + // Identity-stable per-line tap callbacks so `DiffLine`'s memo comparator — + // which compares `onTap` by reference — can hit. A fresh closure per line per + // render defeated it, so tapping one line re-rendered every mounted diff row + // instead of only the rows whose `isSelected` changed. Mirrors + // `pr-diff-file-navigator.tsx`'s `rowCallbacksRef`: the closures read the + // latest item and the latest `onLineTap` through refs, so they stay + // identity-stable (the memo keeps hitting) but never go stale when the item + // is rebuilt or `onLineTap` changes identity. Each row releases its entry + // when FlashList unmounts it (see `DiffLineRow`), so the maps stay bounded by + // the rows actually mounted instead of every line ever rendered. + const onLineTapRef = useRef(onLineTap); + onLineTapRef.current = onLineTap; + + const lineItemRef = useRef(new Map()); + const lineTapRef = useRef(new Map void>()); + + // Bind the row's current item to its line key. Called on every render of a + // mounted line, so the cached callback always reads the current item. + const registerLineItem = useCallback((item: DiffLineItem) => { + lineItemRef.current.set(item.key, item); + }, []); + + // Release both maps when a row unmounts. FlashList unmounts cells as they + // scroll out of its render window; without this, one entry per line ever + // rendered would be kept for the screen's lifetime — and each retained + // `DiffLineItem` holds its file's whole parsed patch, so a collapsed or + // refetched file's lines would never be collected. + const releaseLine = useCallback((key: string) => { + lineItemRef.current.delete(key); + lineTapRef.current.delete(key); + }, []); + + const lineTapFor = useCallback( + (item: DiffLineItem) => { + registerLineItem(item); + let onTap = lineTapRef.current.get(item.key); + if (!onTap) { + onTap = () => { + const current = lineItemRef.current.get(item.key); + if (!current) { + return; + } + const side = sideForDiffLineType(current.line.type); + const lineNumber = side === 'LEFT' ? current.line.oldLine : current.line.newLine; + const hunk = current.parsed.hunks[current.hunkIndex]; + if (lineNumber === undefined || !hunk) { + return; + } + onLineTapRef.current({ + filePath: current.filePath, + hunkKey: `${current.filePath}:${current.hunkIndex}`, + side, + line: lineNumber, + text: current.line.text, + hunk, + }); + }; + lineTapRef.current.set(item.key, onTap); + } + return onTap; + }, + [registerLineItem] + ); + return useCallback( ({ item }: { item: ListItem }) => { switch (item.kind) { @@ -123,23 +229,12 @@ export function useDiffRenderItem({ const hunk = item.parsed.hunks[item.hunkIndex]; const isSelectable = item.selectable !== false; return ( - { - onLineTap({ - filePath: item.filePath, - hunkKey: `${item.filePath}:${item.hunkIndex}`, - side, - line: lineNumber, - text: parsedLine.text, - hunk, - }); - } - : undefined + isSelectable && lineNumber !== undefined && hunk ? lineTapFor(item) : undefined } isSelected={ selection !== null && @@ -178,6 +273,16 @@ export function useDiffRenderItem({ } } }, - [viewed, onRetryPage, onFetchAll, handleLoadContext, setExpanded, onLineTap, selection] + [ + viewed, + onRetryPage, + onFetchAll, + handleLoadContext, + setExpanded, + lineTapFor, + registerLineItem, + releaseLine, + selection, + ] ); } diff --git a/apps/mobile/src/components/pr-review/discussion/comment-moderation.tsx b/apps/mobile/src/components/pr-review/discussion/comment-moderation.tsx new file mode 100644 index 00000000000..e7dc5b9608b --- /dev/null +++ b/apps/mobile/src/components/pr-review/discussion/comment-moderation.tsx @@ -0,0 +1,209 @@ +// Shared comment-moderation actions for every mounted comment row. +// +// Each row used to mount four `useMutation` hooks (report content, report user, +// mute, block) that are only reachable from its overflow action sheet: N +// mounted rows held 4N mutation-cache listeners, all notified on every +// mutation anywhere in the app, and rebuilt four option closures per row per +// render. The Discussion tab mounts one `CommentModerationProvider` for the +// whole list and every row reads the same four mutations through +// `useCommentModerationActions`. +// +// The provider is required, not optional: there is no second path in which a +// row still owns mutations, so a row outside the provider is a bug and the +// reader throws rather than falling back to a private set of mutations. + +import { useMutation, useQueryClient } from '@tanstack/react-query'; +import { createContext, type ReactNode, useContext, useMemo } from 'react'; +import { useTranslation } from 'react-i18next'; + +import { i18n } from '@/i18n'; +import { announcingToast } from '@/lib/a11y/announcing-toast'; +import { useTRPC } from '@/lib/trpc'; +import { isTerminalTrpcCode, readTrpcErrorField } from '@/lib/trpc-error'; +import { Alert } from 'react-native'; + +const REPORT_PLATFORM = 'mobile'; + +type ModerationAction = 'report-content' | 'report-user' | 'mute' | 'block'; + +type ModerationFailure = + | { kind: 'terminal'; message: string } + | { kind: 'retryable'; message: string }; + +const TERMINAL_KEYS = { + 'report-content': 'prReview.discussion.moderation.reportContent.terminal', + 'report-user': 'prReview.discussion.moderation.reportUser.terminal', + mute: 'prReview.discussion.moderation.mute.terminal', + block: 'prReview.discussion.moderation.block.terminal', +} as const satisfies Record; + +const RETRYABLE_KEYS = { + 'report-content': 'prReview.discussion.moderation.reportContent.retryable', + 'report-user': 'prReview.discussion.moderation.reportUser.retryable', + mute: 'prReview.discussion.moderation.mute.retryable', + block: 'prReview.discussion.moderation.block.retryable', +} as const satisfies Record; + +/** Terminal moderation failures must not be retried; everything else is retryable. */ +export function moderationFailure(action: ModerationAction, error: unknown): ModerationFailure { + const code = readTrpcErrorField(error, 'code'); + if (isTerminalTrpcCode(code)) { + return { kind: 'terminal', message: i18n.t(TERMINAL_KEYS[action]) }; + } + return { kind: 'retryable', message: i18n.t(RETRYABLE_KEYS[action]) }; +} + +/** Terminal failures toast once; retryable failures offer a Retry CTA. */ +function showModerationFailure(action: ModerationAction, error: unknown, retry: () => void): void { + const failure = moderationFailure(action, error); + if (failure.kind === 'terminal') { + announcingToast.error(failure.message); + return; + } + Alert.alert(i18n.t('common.somethingWentWrong'), failure.message, [ + { text: i18n.t('common.cancel'), style: 'cancel' }, + { text: i18n.t('common.retry'), onPress: retry }, + ]); +} + +/** + * One moderation request, discriminated so a row names the action and its + * target instead of reaching for a specific mutation. + */ +type CommentModerationRequest = + | { readonly action: 'report-content'; readonly commentId: number } + | { readonly action: 'report-user'; readonly githubLogin: string } + | { readonly action: 'mute'; readonly githubLogin: string } + | { readonly action: 'block'; readonly githubLogin: string }; + +export type CommentModeration = { + readonly report: (request: CommentModerationRequest) => void; +}; + +/** + * Build the four moderation mutations once and hand out a single `report` + * dispatcher. The declaration order (reportContent, reportUser, blockUser, + * muteUser) is the order the tests index the captured mutation options in. + */ +function useCommentModeration(): CommentModeration { + const { t } = useTranslation(); + const trpc = useTRPC(); + const queryClient = useQueryClient(); + + const invalidateHiddenUsers = () => { + void queryClient.invalidateQueries({ queryKey: trpc.moderation.listHiddenUsers.queryKey() }); + }; + + const { mutate: reportContent } = useMutation( + trpc.moderation.reportContent.mutationOptions({ + onSuccess: result => + announcingToast.success( + t('common.reportSubmittedReceipt', { receiptId: result.receiptId }) + ), + onError: (error, variables) => { + showModerationFailure('report-content', error, () => { + reportContent(variables); + }); + }, + }) + ); + const { mutate: reportUser } = useMutation( + trpc.moderation.reportUser.mutationOptions({ + onSuccess: result => + announcingToast.success( + t('common.reportSubmittedReceipt', { receiptId: result.receiptId }) + ), + onError: (error, variables) => { + showModerationFailure('report-user', error, () => { + reportUser(variables); + }); + }, + }) + ); + const { mutate: blockUser } = useMutation( + trpc.moderation.blockUser.mutationOptions({ + onSuccess: (_result, input) => { + invalidateHiddenUsers(); + announcingToast.success(t('prReview.discussion.blockedUser', { login: input.githubLogin })); + }, + onError: (error, variables) => { + showModerationFailure('block', error, () => { + blockUser(variables); + }); + }, + }) + ); + const { mutate: muteUser } = useMutation( + trpc.moderation.muteUser.mutationOptions({ + onSuccess: (_result, input) => { + invalidateHiddenUsers(); + announcingToast.success(t('prReview.discussion.mutedUser', { login: input.githubLogin })); + }, + onError: (error, variables) => { + showModerationFailure('mute', error, () => { + muteUser(variables); + }); + }, + }) + ); + + // Key on the four `mutate` functions: TanStack returns a fresh result object + // per render but the `mutate` callbacks are stable, so keying on the object + // would rebuild the value (and re-render every row holding it) every render. + return useMemo( + () => ({ + report: request => { + switch (request.action) { + case 'report-content': { + reportContent({ + surface: 'pr_discussion_content', + targetKind: 'comment', + targetId: String(request.commentId), + reason: 'other', + context: { platform: REPORT_PLATFORM }, + }); + break; + } + case 'report-user': { + reportUser({ targetId: request.githubLogin, reason: 'other' }); + break; + } + case 'mute': { + muteUser({ githubLogin: request.githubLogin }); + break; + } + case 'block': { + blockUser({ githubLogin: request.githubLogin }); + break; + } + default: { + // Unreachable: `request.action` is the exhaustive discriminated union. + break; + } + } + }, + }), + [blockUser, muteUser, reportContent, reportUser] + ); +} + +const CommentModerationContext = createContext(null); + +/** Provide the shared moderation actions to every comment row under it. */ +export function CommentModerationProvider({ children }: { readonly children: ReactNode }) { + const moderation = useCommentModeration(); + return ( + + {children} + + ); +} + +/** Read the provider's shared moderation actions; throws without a provider. */ +export function useCommentModerationActions(): CommentModeration { + const moderation = useContext(CommentModerationContext); + if (moderation === null) { + throw new Error('useCommentModerationActions must be used inside a CommentModerationProvider'); + } + return moderation; +} diff --git a/apps/mobile/src/components/pr-review/discussion/comment-row.test.tsx b/apps/mobile/src/components/pr-review/discussion/comment-row.test.tsx index 6d99bf9cd79..7c6fccd31b9 100644 --- a/apps/mobile/src/components/pr-review/discussion/comment-row.test.tsx +++ b/apps/mobile/src/components/pr-review/discussion/comment-row.test.tsx @@ -1,8 +1,10 @@ +/* eslint-disable max-lines -- one suite for the row's moderation overflow, its capability gates, its CTA wiring and the shared-mutation provider (Fix 19) */ import { createElement } from 'react'; import { act, TestRenderer } from '@/test/renderer'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { CommentRow, moderationFailure } from './comment-row'; +import { CommentRow } from './comment-row'; +import { CommentModerationProvider, moderationFailure } from './comment-moderation'; import { type ReviewComment } from '@/lib/pr-review/discussion/review-discussion-types'; // ── Fixture ────────────────────────────────────────────────────────── @@ -114,16 +116,20 @@ async function render( await act(async () => { await Promise.resolve(); renderer = TestRenderer.create( - createElement(CommentRow, { - comment, - owner: 'octocat', - repo: 'hello', - number: 7, - commentKind: 'review', - onToggleReaction: vi.fn<() => void>(), - viewerLogin, - ...extra, - }) + createElement( + CommentModerationProvider, + null, + createElement(CommentRow, { + comment, + owner: 'octocat', + repo: 'hello', + number: 7, + commentKind: 'review', + onToggleReaction: vi.fn<() => void>(), + viewerLogin, + ...extra, + }) + ) ); }); // eslint-disable-next-line typescript-eslint/no-unnecessary-condition @@ -133,6 +139,36 @@ async function render( return renderer; } +/** Mounts `count` rows under ONE provider (Fix 19: the mutations are shared). */ +async function renderRows(count: number): Promise { + let renderer: TestRenderer.ReactTestRenderer | null = null; + await act(async () => { + await Promise.resolve(); + renderer = TestRenderer.create( + createElement( + CommentModerationProvider, + null, + ...Array.from({ length: count }, (_, index) => + createElement(CommentRow, { + comment: makeComment({ commentId: index + 1, nodeId: `C${index + 1}` }), + owner: 'octocat', + repo: 'hello', + number: 7, + commentKind: 'review', + onToggleReaction: vi.fn<() => void>(), + viewerLogin: 'bob', + }) + ) + ) + ); + }); + // eslint-disable-next-line typescript-eslint/no-unnecessary-condition -- act() may not assign + if (!renderer) { + throw new Error('Failed to create test renderer'); + } + return renderer; +} + function openOverflow(renderer: TestRenderer.ReactTestRenderer): void { const trigger = renderer.root.find( node => @@ -310,6 +346,30 @@ describe('CommentRow overflow actions', () => { }); }); +// Fix 19: the four moderation `useMutation` hooks used to mount once per row +// (4N observers for N mounted rows). One provider owns them and every row +// shares that set. +describe('CommentModerationProvider sharing (Fix 19)', () => { + beforeEach(() => { + mutateFns.length = 0; + capturedOptions.length = 0; + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + it('mounts four mutations once for a provider holding several rows, not once per row', async () => { + const renderer = await renderRows(3); + + expect(renderer.root.findAll(node => String(node.type) === 'MarkdownText')).toHaveLength(3); + expect(mutateFns).toHaveLength(4); + expect(capturedOptions).toHaveLength(4); + + renderer.unmount(); + }); +}); + describe('CommentRow reactions capability gate (s6)', () => { afterEach(() => { vi.clearAllMocks(); diff --git a/apps/mobile/src/components/pr-review/discussion/comment-row.touch-target.test.tsx b/apps/mobile/src/components/pr-review/discussion/comment-row.touch-target.test.tsx index 5753383a0c5..5fba6cae068 100644 --- a/apps/mobile/src/components/pr-review/discussion/comment-row.touch-target.test.tsx +++ b/apps/mobile/src/components/pr-review/discussion/comment-row.touch-target.test.tsx @@ -8,6 +8,7 @@ import { act, TestRenderer } from '@/test/renderer'; import { describe, expect, it, vi } from 'vitest'; import { CommentRow } from './comment-row'; +import { CommentModerationProvider } from './comment-moderation'; import { MIN_AUDITED_CONTROL_FRAME_DP } from '@/lib/a11y/touch-target'; import { COMMENT_ACTIONS_FRAME_DP, @@ -81,15 +82,19 @@ async function render(): Promise { await act(async () => { await Promise.resolve(); renderer = TestRenderer.create( - createElement(CommentRow, { - comment, - owner: 'octocat', - repo: 'hello', - number: 7, - commentKind: 'review', - onToggleReaction: vi.fn<() => void>(), - viewerLogin: 'bob', - }) + createElement( + CommentModerationProvider, + null, + createElement(CommentRow, { + comment, + owner: 'octocat', + repo: 'hello', + number: 7, + commentKind: 'review', + onToggleReaction: vi.fn<() => void>(), + viewerLogin: 'bob', + }) + ) ); }); // eslint-disable-next-line typescript-eslint/no-unnecessary-condition -- act() may not assign diff --git a/apps/mobile/src/components/pr-review/discussion/comment-row.tsx b/apps/mobile/src/components/pr-review/discussion/comment-row.tsx index a3a6c0f859d..6900c5e4f3a 100644 --- a/apps/mobile/src/components/pr-review/discussion/comment-row.tsx +++ b/apps/mobile/src/components/pr-review/discussion/comment-row.tsx @@ -21,18 +21,16 @@ // and renders nothing when the comment has no addressable URL. import { useActionSheet } from '@expo/react-native-action-sheet'; -import { useMutation, useQueryClient } from '@tanstack/react-query'; import { useTranslation } from 'react-i18next'; import { useSafeAreaInsets } from 'react-native-safe-area-context'; import { MarkdownText } from '@/components/agents/markdown-text'; +import { useCommentModerationActions } from '@/components/pr-review/discussion/comment-moderation'; +import { PrCommentFixWithKilo } from '@/components/pr-review/discussion/pr-comment-fix-with-kilo'; +import { ReactionsRow } from '@/components/pr-review/discussion/reactions-row'; import { MoreHorizontal } from '@/components/ui/icons'; import { Image } from '@/components/ui/image'; import { Text } from '@/components/ui/text'; -import { PrCommentFixWithKilo } from '@/components/pr-review/discussion/pr-comment-fix-with-kilo'; -import { ReactionsRow } from '@/components/pr-review/discussion/reactions-row'; -import { i18n } from '@/i18n'; -import { announcingToast } from '@/lib/a11y/announcing-toast'; import { useThemeColors } from '@/lib/hooks/use-theme-colors'; import { COMMENT_ACTIONS_HIT_SLOP } from '@/lib/pr-review/comment-trailing-controls'; import { type PrCommentKind } from '@/lib/pr-review/fix-with-kilo'; @@ -41,10 +39,8 @@ import { type ReviewReactionContent, selectCommentAuthorName, } from '@/lib/pr-review/discussion/review-discussion-types'; -import { useTRPC } from '@/lib/trpc'; -import { isTerminalTrpcCode, readTrpcErrorField } from '@/lib/trpc-error'; import { parseTimestamp, timeAgo } from '@/lib/utils'; -import { Alert, Pressable, View } from 'react-native'; +import { Pressable, View } from 'react-native'; type CommentRowProps = { readonly comment: ReviewComment; @@ -68,50 +64,6 @@ type CommentRowProps = { readonly viewerLogin?: string | null; }; -const REPORT_PLATFORM = 'mobile'; - -type ModerationAction = 'report-content' | 'report-user' | 'mute' | 'block'; - -type ModerationFailure = - | { kind: 'terminal'; message: string } - | { kind: 'retryable'; message: string }; - -const TERMINAL_KEYS = { - 'report-content': 'prReview.discussion.moderation.reportContent.terminal', - 'report-user': 'prReview.discussion.moderation.reportUser.terminal', - mute: 'prReview.discussion.moderation.mute.terminal', - block: 'prReview.discussion.moderation.block.terminal', -} as const satisfies Record; - -const RETRYABLE_KEYS = { - 'report-content': 'prReview.discussion.moderation.reportContent.retryable', - 'report-user': 'prReview.discussion.moderation.reportUser.retryable', - mute: 'prReview.discussion.moderation.mute.retryable', - block: 'prReview.discussion.moderation.block.retryable', -} as const satisfies Record; - -/** Terminal moderation failures must not be retried; everything else is retryable. */ -export function moderationFailure(action: ModerationAction, error: unknown): ModerationFailure { - const code = readTrpcErrorField(error, 'code'); - if (isTerminalTrpcCode(code)) { - return { kind: 'terminal', message: i18n.t(TERMINAL_KEYS[action]) }; - } - return { kind: 'retryable', message: i18n.t(RETRYABLE_KEYS[action]) }; -} - -/** Terminal failures toast once; retryable failures offer a Retry CTA. */ -function showModerationFailure(action: ModerationAction, error: unknown, retry: () => void): void { - const failure = moderationFailure(action, error); - if (failure.kind === 'terminal') { - announcingToast.error(failure.message); - return; - } - Alert.alert(i18n.t('common.somethingWentWrong'), failure.message, [ - { text: i18n.t('common.cancel'), style: 'cancel' }, - { text: i18n.t('common.retry'), onPress: retry }, - ]); -} - export function CommentRow({ comment, onToggleReaction, @@ -131,65 +83,7 @@ export function CommentRow({ const { t } = useTranslation(); const { bottom } = useSafeAreaInsets(); const { showActionSheetWithOptions } = useActionSheet(); - const trpc = useTRPC(); - const queryClient = useQueryClient(); - - const invalidateHiddenUsers = () => { - void queryClient.invalidateQueries({ queryKey: trpc.moderation.listHiddenUsers.queryKey() }); - }; - - const reportContent = useMutation( - trpc.moderation.reportContent.mutationOptions({ - onSuccess: result => - announcingToast.success( - t('common.reportSubmittedReceipt', { receiptId: result.receiptId }) - ), - onError: (error, variables) => { - showModerationFailure('report-content', error, () => { - reportContent.mutate(variables); - }); - }, - }) - ); - const reportUser = useMutation( - trpc.moderation.reportUser.mutationOptions({ - onSuccess: result => - announcingToast.success( - t('common.reportSubmittedReceipt', { receiptId: result.receiptId }) - ), - onError: (error, variables) => { - showModerationFailure('report-user', error, () => { - reportUser.mutate(variables); - }); - }, - }) - ); - const blockUser = useMutation( - trpc.moderation.blockUser.mutationOptions({ - onSuccess: (_result, input) => { - invalidateHiddenUsers(); - announcingToast.success(t('prReview.discussion.blockedUser', { login: input.githubLogin })); - }, - onError: (error, variables) => { - showModerationFailure('block', error, () => { - blockUser.mutate(variables); - }); - }, - }) - ); - const muteUser = useMutation( - trpc.moderation.muteUser.mutationOptions({ - onSuccess: (_result, input) => { - invalidateHiddenUsers(); - announcingToast.success(t('prReview.discussion.mutedUser', { login: input.githubLogin })); - }, - onError: (error, variables) => { - showModerationFailure('mute', error, () => { - muteUser.mutate(variables); - }); - }, - }) - ); + const moderation = useCommentModerationActions(); function openOverflow() { const author = comment.author; @@ -204,19 +98,19 @@ export function CommentRow({ userActions.push({ label: t('prReview.discussion.reportUser'), run: () => { - reportUser.mutate({ targetId: author.login, reason: 'other' }); + moderation.report({ action: 'report-user', githubLogin: author.login }); }, }); userActions.push({ label: t('prReview.discussion.mute'), run: () => { - muteUser.mutate({ githubLogin: author.login }); + moderation.report({ action: 'mute', githubLogin: author.login }); }, }); userActions.push({ label: t('prReview.discussion.block'), run: () => { - blockUser.mutate({ githubLogin: author.login }); + moderation.report({ action: 'block', githubLogin: author.login }); }, }); } @@ -238,13 +132,7 @@ export function CommentRow({ return; } if (index === 0) { - reportContent.mutate({ - surface: 'pr_discussion_content', - targetKind: 'comment', - targetId: String(comment.commentId), - reason: 'other', - context: { platform: REPORT_PLATFORM }, - }); + moderation.report({ action: 'report-content', commentId: comment.commentId }); return; } userActions[index - 1]?.run(); diff --git a/apps/mobile/src/components/pr-review/pr-review-discussion-tab.test-helpers.tsx b/apps/mobile/src/components/pr-review/pr-review-discussion-tab.test-helpers.tsx index 923aa100226..c6dad8ffdb8 100644 --- a/apps/mobile/src/components/pr-review/pr-review-discussion-tab.test-helpers.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-discussion-tab.test-helpers.tsx @@ -82,6 +82,12 @@ vi.mock('@/components/ui/icons', () => ({ MessageSquarePlus: 'MessageSquarePlus' vi.mock('@/components/pr-review/discussion/pr-review-discussion-list', () => ({ PrReviewDiscussionList: 'PrReviewDiscussionList', })); +// The tab mounts the shared moderation provider around the list; here it is a +// plain host node so the tab suite can assert the nesting (the provider's own +// behaviour is covered by comment-row.test.tsx, which mounts the real one). +vi.mock('@/components/pr-review/discussion/comment-moderation', () => ({ + CommentModerationProvider: 'CommentModerationProvider', +})); vi.mock('@/components/pr-review/discussion/pr-comment-cta', () => ({ PrCommentCta: 'PrCommentCta', })); @@ -114,6 +120,14 @@ export function mountTab(scopeRef?: ProviderPrRef): TestRenderer.ReactTestRender return renderer; } +/** Re-renders the mounted tab in place with the same props, so a test can + * compare a prop's identity across two renders of one component instance. */ +export function rerenderTab(renderer: TestRenderer.ReactTestRenderer): void { + act(() => { + renderer.update(createElement(PrReviewDiscussionTab, BASE_PROPS)); + }); +} + export function bottomPaddedViews( renderer: TestRenderer.ReactTestRenderer ): TestRenderer.ReactTestInstance[] { diff --git a/apps/mobile/src/components/pr-review/pr-review-discussion-tab.test.tsx b/apps/mobile/src/components/pr-review/pr-review-discussion-tab.test.tsx index 4f0affd8399..67045497995 100644 --- a/apps/mobile/src/components/pr-review/pr-review-discussion-tab.test.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-discussion-tab.test.tsx @@ -12,6 +12,7 @@ import { mountTab, pushMock, replyScrollFns, + rerenderTab, resetState, } from './pr-review-discussion-tab.test-helpers'; import { act, type ReactTestRenderer } from '@/test/renderer'; @@ -334,3 +335,55 @@ describe('PrReviewDiscussionTab keyboard-lift gating and reply-scroll wiring', ( expect(replyScrollFns.invalidate).toHaveBeenCalledTimes(1); }); }); + +// Fix 11: the merge is memoized on its two identity-stable inputs, so a +// re-render with unchanged data hands FlashList the same array instead of a +// freshly merged and sorted one (which would re-diff the whole list). +describe('PrReviewDiscussionTab merged list memoization (Fix 11)', () => { + it('reuses the same listItems array across a re-render with unchanged inputs', () => { + discussionState.conversation = [{ nodeId: 'c1', createdAt: null }]; + const renderer = mountTab(); + + const first = renderer.root.find(node => String(node.type) === 'PrReviewDiscussionList'); + const firstItems = first.props.listItems; + + rerenderTab(renderer); + + const second = renderer.root.find(node => String(node.type) === 'PrReviewDiscussionList'); + expect(second.props.listItems).toBe(firstItems); + + renderer.unmount(); + }); + + it('reuses the same listItems array when the hook hands back a fresh empty conversation each render', () => { + // The real hook returns a brand-new `[]` for a PR whose first page carries + // no conversation comments: `retainConversationAcrossMounts` falls back to + // a fresh literal per call. A memo keyed on that raw array never hits, so + // the tab substitutes a stable empty reference. + discussionState.threads = [{ threadId: 'T1', isResolved: false, comments: [] }]; + discussionState.conversation = []; + const renderer = mountTab(); + + const first = renderer.root.find(node => String(node.type) === 'PrReviewDiscussionList'); + const firstItems = first.props.listItems; + + // A NEW empty array, exactly like the hook's next call. + discussionState.conversation = []; + rerenderTab(renderer); + + const second = renderer.root.find(node => String(node.type) === 'PrReviewDiscussionList'); + expect(second.props.listItems).toBe(firstItems); + + renderer.unmount(); + }); + + it('mounts the happy list inside the shared comment-moderation provider', () => { + discussionState.conversation = [{ nodeId: 'c1', createdAt: null }]; + const renderer = mountTab(); + + const provider = renderer.root.find(node => String(node.type) === 'CommentModerationProvider'); + expect(provider.find(node => String(node.type) === 'PrReviewDiscussionList')).toBeDefined(); + + renderer.unmount(); + }); +}); diff --git a/apps/mobile/src/components/pr-review/pr-review-discussion-tab.tsx b/apps/mobile/src/components/pr-review/pr-review-discussion-tab.tsx index abdc27ff9a0..b86d70dde10 100644 --- a/apps/mobile/src/components/pr-review/pr-review-discussion-tab.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-discussion-tab.tsx @@ -55,11 +55,12 @@ import { type FlashListRef } from '@shopify/flash-list'; import { MessageSquarePlus } from '@/components/ui/icons'; import { type Href, useIsFocused, useRouter } from 'expo-router'; -import { type ReactNode, useEffect, useRef, useState } from 'react'; +import { type ReactNode, useEffect, useMemo, useRef, useState } from 'react'; import { useTranslation } from 'react-i18next'; import { Platform, View } from 'react-native'; import { useSafeAreaInsets } from 'react-native-safe-area-context'; +import { CommentModerationProvider } from '@/components/pr-review/discussion/comment-moderation'; import { PrReviewDiscussionList } from '@/components/pr-review/discussion/pr-review-discussion-list'; import { PrCommentCta } from '@/components/pr-review/discussion/pr-comment-cta'; import { providerPrSheetHref } from '@/components/pr-review/pr-review-provider-sheet-href'; @@ -71,11 +72,12 @@ import { Button } from '@/components/ui/button'; import { Skeleton } from '@/components/ui/skeleton'; import { Text } from '@/components/ui/text'; import { + type ConversationComment, type DiscussionListItem, isDiscussionEmpty, - mergeDiscussionListItems, type ReviewThread, } from '@/lib/pr-review/discussion/review-discussion-types'; +import { mergeDiscussionListItemsBySortKey } from '@/lib/pr-review/discussion/merge-discussion-list-items'; import { useMotionPolicy } from '@/lib/a11y/motion'; import { expandedForThread, @@ -103,6 +105,15 @@ type PrReviewDiscussionTabProps = { const SKELETON_ROW_COUNT = 4; +// One shared empty reference for the common "first page carries no +// conversation comments" case. `retainConversationAcrossMounts` falls back to +// a brand-new `[]` literal on every call when nothing was retained, so a memo +// keyed on the raw `conversation` would re-merge and re-sort on every render of +// such a PR. Substituting this reference keeps the key stable when the set is +// empty; a non-empty conversation is already identity-stable (it is read from +// the memoized first page). +const EMPTY_CONVERSATION: readonly ConversationComment[] = []; + // The GitHub conversation-comment formSheet. GitLab and Bitbucket reach their // own sibling route inside the provider layout (providerPrSheetHref) so // the sheet mounts under the live provider scope; GitHub keeps the exact @@ -163,6 +174,19 @@ export function PrReviewDiscussionTab({ // className). Same treatment as the diff floating-actions bar. const insets = useSafeAreaInsets(); + // Full re-sort of every loaded thread + first-page conversation comments + // (R4: "Load more" may insert rows mid-list; accepted). Both inputs are + // identity-stable (`threads` is memoized on `pages`; `conversation` is the + // retained first page, or EMPTY_CONVERSATION when the PR has none), so + // memoize the merge: an expand tap, optimistic reaction or reply focus must + // not re-merge and re-sort the whole loaded set. + // The merge parses each item's timestamp once (merge-discussion-list-items.ts). + const stableConversation = conversation.length === 0 ? EMPTY_CONVERSATION : conversation; + const listItems = useMemo( + () => mergeDiscussionListItemsBySortKey(threads, stableConversation), + [threads, stableConversation] + ); + // Single write path: the ref is the tap-time source of truth (render-closure // state can lag a queued update on rapid taps). const applyExpansion = (next: Record) => { @@ -396,37 +420,35 @@ export function PrReviewDiscussionTab({ } // ── Happy / paginated list ───────────────────────────────────────── - // Full re-sort of every loaded thread + first-page conversation - // comments (R4: "Load more" may insert rows mid-list; accepted). - const listItems = mergeDiscussionListItems(threads, conversation); - return withCommentCta( - { - invalidateSettle(); - // A user drag wins over the keyboard-open reply park: drop the - // pending focus scroll (useReplyFocusScroll). - replyScroll.invalidate(); - }} - hasNextPage={query.hasNextPage} - isFetchingNextPage={query.isFetchingNextPage} - laterPageError={laterPageError || retainedContentError} - onLoadMore={() => { - void query.fetchNextPage(); - }} - onRetryLoadMore={() => { - void query.refetch(); - }} - onReplyInputFocus={handleReplyInputFocus} - onViewportLayout={handleViewportLayout} - /> + + { + invalidateSettle(); + // A user drag wins over the keyboard-open reply park: drop the + // pending focus scroll (useReplyFocusScroll). + replyScroll.invalidate(); + }} + hasNextPage={query.hasNextPage} + isFetchingNextPage={query.isFetchingNextPage} + laterPageError={laterPageError || retainedContentError} + onLoadMore={() => { + void query.fetchNextPage(); + }} + onRetryLoadMore={() => { + void query.refetch(); + }} + onReplyInputFocus={handleReplyInputFocus} + onViewportLayout={handleViewportLayout} + /> + ); } diff --git a/apps/mobile/src/lib/pr-review/diff/pr-diff-list-builder.test.ts b/apps/mobile/src/lib/pr-review/diff/pr-diff-list-builder.test.ts index 68ca0731adc..31200ca1e5a 100644 --- a/apps/mobile/src/lib/pr-review/diff/pr-diff-list-builder.test.ts +++ b/apps/mobile/src/lib/pr-review/diff/pr-diff-list-builder.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it } from 'vitest'; +import { describe, expect, it, vi } from 'vitest'; import { buildFileItems, @@ -6,8 +6,15 @@ import { buildPaginationItem, } from '@/lib/pr-review/diff/pr-diff-list-builder'; import { type BuildItemsArgs, type ListItem } from '@/lib/pr-review/diff/pr-diff-list-items'; +import { parsePatch } from '@/lib/pr-review/diff/parse-patch'; import { type PrReviewFile } from '@/lib/pr-review/diff/pr-review-file-types'; +// Wraps the real parser in a spy so a test can count parse calls. +vi.mock('@/lib/pr-review/diff/parse-patch', async importOriginal => { + const actual = await importOriginal<{ parsePatch: typeof parsePatch }>(); + return { ...actual, parsePatch: vi.fn(actual.parsePatch) }; +}); + type FilePatchMissingItem = Extract; type SeparatorItem = Extract; @@ -309,3 +316,15 @@ describe('buildItems composition', () => { expect(composed).toEqual(direct); }); }); + +describe('buildFileItems parsed patch cache', () => { + it('parses each file object once across two builds that differ only in viewed', () => { + const spy = vi.mocked(parsePatch); + spy.mockClear(); + const args = baseArgs({ files: [makeFile(singleHunkPatch)], expanded: { 'a.ts': true } }); + const build = (next: BuildItemsArgs) => + buildFileItems(next).find((i): i is DiffLineListItem => i.kind === 'diff-line'); + expect(build(args)?.parsed).toBe(build({ ...args, viewed: () => true })?.parsed); + expect(spy).toHaveBeenCalledTimes(1); + }); +}); diff --git a/apps/mobile/src/lib/pr-review/diff/pr-diff-list-builder.ts b/apps/mobile/src/lib/pr-review/diff/pr-diff-list-builder.ts index fd53a004e76..bb87cf87f88 100644 --- a/apps/mobile/src/lib/pr-review/diff/pr-diff-list-builder.ts +++ b/apps/mobile/src/lib/pr-review/diff/pr-diff-list-builder.ts @@ -19,6 +19,24 @@ import { buildSideBySideRows } from '@/lib/pr-review/diff/side-by-side'; type ParsedFile = ReturnType; type ParsedHunk = ParsedFile['hunks'][number]; +// Parsed patches keyed by file-object identity. `buildFileItems` re-runs on +// every `viewed` / `expanded` change, and the file DTOs come straight from the +// query pages and are immutable for their lifetime, so a WeakMap cannot go +// stale and is released with the pages when a refetch replaces them. Returning +// the same `ParsedFile` object across rebuilds also keeps `item.parsed` / +// `item.line` reference-stable so `DiffLine`'s memo comparator can hit. +const parsedPatchCache = new WeakMap(); + +function parsedPatchFor(file: BuildItemsArgs['files'][number]): ParsedFile { + const cached = parsedPatchCache.get(file); + if (cached) { + return cached; + } + const parsed: ParsedFile = file.patch ? parsePatch(file.patch) : { isRename: false, hunks: [] }; + parsedPatchCache.set(file, parsed); + return parsed; +} + function pushSideBySideHunk(args: { items: ListItem[]; file: BuildItemsArgs['files'][number]; @@ -115,7 +133,7 @@ function pushExpandedFileItems( number: args.number, path: file.path, }); - const parsed: ParsedFile = file.patch ? parsePatch(file.patch) : { isRename: false, hunks: [] }; + const parsed: ParsedFile = parsedPatchFor(file); const hunks = parsed.hunks; if (file.patchMissing || hunks.length === 0) { diff --git a/apps/mobile/src/lib/pr-review/discussion/merge-discussion-list-items.test.ts b/apps/mobile/src/lib/pr-review/discussion/merge-discussion-list-items.test.ts new file mode 100644 index 00000000000..07cfab64715 --- /dev/null +++ b/apps/mobile/src/lib/pr-review/discussion/merge-discussion-list-items.test.ts @@ -0,0 +1,133 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { parseTimestamp } from '@/lib/utils'; + +import { mergeDiscussionListItemsBySortKey } from './merge-discussion-list-items'; +import { + type ConversationComment, + type DiscussionListItem, + mergeDiscussionListItems, + type ReviewComment, + type ReviewThread, +} from './review-discussion-types'; + +// Count every parse while delegating to the real implementation. The whole +// point of the decorated merge is that no comparator call parses a timestamp. +vi.mock('@/lib/utils', async importOriginal => { + const actual = await importOriginal<{ parseTimestamp: (value: string) => Date }>(); + return { ...actual, parseTimestamp: vi.fn(actual.parseTimestamp) }; +}); + +function at(offsetSeconds: number): string { + return new Date(Date.UTC(2024, 0, 1, 0, 0, offsetSeconds)).toISOString(); +} + +function makeComment(overrides: Partial = {}): ReviewComment { + return { + commentId: 1, + nodeId: 'C1', + author: { login: 'alice', avatarUrl: 'https://example.com/a.png' }, + bodyMarkdown: 'hello', + createdAt: '2024-01-01T00:00:00Z', + reactions: [{ content: 'THUMBS_UP', count: 2, viewerHasReacted: false }], + ...overrides, + }; +} + +function makeThread(overrides: Partial = {}): ReviewThread { + return { + threadId: 'T1', + isResolved: false, + isOutdated: false, + subjectType: 'LINE', + path: 'src/index.ts', + line: 10, + startLine: null, + originalLine: null, + originalStartLine: null, + diffSide: 'RIGHT', + diffHunk: null, + comments: [makeComment()], + ...overrides, + }; +} + +function makeConversation(overrides: Partial = {}): ConversationComment { + return makeComment({ nodeId: 'IC1', commentId: 100, ...overrides }); +} + +function threadAt(threadId: string, firstAt: string): ReviewThread { + return makeThread({ + threadId, + comments: [makeComment({ nodeId: `${threadId}-1`, createdAt: firstAt })], + }); +} + +function keysOf(items: readonly DiscussionListItem[]): string[] { + return items.map(item => + item.kind === 'thread' ? `thread:${item.thread.threadId}` : `comment:${item.comment.nodeId}` + ); +} + +const parseTimestampMock = vi.mocked(parseTimestamp); + +describe('mergeDiscussionListItemsBySortKey', () => { + beforeEach(() => { + parseTimestampMock.mockClear(); + }); + + it('orders exactly like mergeDiscussionListItems for a tie and a missing timestamp', () => { + const tieStamp = at(3); + const threads = [ + threadAt('T1', tieStamp), + threadAt('T2', at(1)), + makeThread({ threadId: 'T3', comments: [makeComment({ nodeId: 'T3-1', createdAt: '' })] }), + ]; + const conversation: ConversationComment[] = [ + makeConversation({ nodeId: 'IC-tie', createdAt: tieStamp }), + makeConversation({ nodeId: 'IC-late', createdAt: at(9) }), + ]; + + const bySortKey = mergeDiscussionListItemsBySortKey(threads, conversation); + const reference = mergeDiscussionListItems(threads, conversation); + + // Equal createdAt (thread vs conversation) breaks to the thread; the empty + // timestamp sorts last. + expect(keysOf(bySortKey)).toEqual([ + 'thread:T2', + 'thread:T1', + 'comment:IC-tie', + 'comment:IC-late', + 'thread:T3', + ]); + expect(keysOf(bySortKey)).toEqual(keysOf(reference)); + }); + + it('parses each item with a timestamp once per merge, not once per comparison', () => { + const threads = [ + threadAt('A', at(1)), + threadAt('B', at(3)), + // No comments → no timestamp to parse. + makeThread({ threadId: 'C', comments: [] }), + ]; + const conversation: ConversationComment[] = [ + makeConversation({ nodeId: 'IC-d', createdAt: at(2) }), + // Empty timestamp → not parsed. + makeConversation({ nodeId: 'IC-e', createdAt: '' }), + ]; + + const merged = mergeDiscussionListItemsBySortKey(threads, conversation); + + expect(keysOf(merged)).toEqual([ + 'thread:A', + 'comment:IC-d', + 'thread:B', + 'thread:C', + 'comment:IC-e', + ]); + // One parse per timestamped item, in decoration (input) order; the + // timestamp-less items are never parsed. + expect(parseTimestampMock.mock.calls.map(([raw]) => raw)).toEqual([at(1), at(3), at(2)]); + expect(parseTimestampMock).toHaveBeenCalledTimes(3); + }); +}); diff --git a/apps/mobile/src/lib/pr-review/discussion/merge-discussion-list-items.ts b/apps/mobile/src/lib/pr-review/discussion/merge-discussion-list-items.ts new file mode 100644 index 00000000000..ad1712be58b --- /dev/null +++ b/apps/mobile/src/lib/pr-review/discussion/merge-discussion-list-items.ts @@ -0,0 +1,86 @@ +// One-pass decorated form of the Discussion tab's merge/sort. +// +// `mergeDiscussionListItems` sorts with `compareDiscussionListItems`, whose +// comparator reads (and `parseTimestamp`s) each item's raw timestamp on every +// comparison — twice per comparison, so thousands of parses per merge on a PR +// with a few hundred loaded rows. Both inputs to the tab's merge are +// identity-stable, but the tab re-renders on every expand/collapse tap, +// optimistic reaction/resolve and reply-focus event, so that cost lands on the +// interaction that caused the render. +// +// This module reads each item's raw timestamp once, carries a precomputed +// `{ hasMs, ms }` key, sorts on that key, and delegates exact ties to +// `compareDiscussionListItems` so the A2.2 total order is identical to +// `mergeDiscussionListItems` for every input. + +import { + compareDiscussionListItems, + type ConversationComment, + type DiscussionListItem, + type ReviewThread, +} from '@/lib/pr-review/discussion/review-discussion-types'; +import { parseTimestamp } from '@/lib/utils'; + +type DecoratedDiscussionListItem = { + readonly item: DiscussionListItem; + /** False when the item has no usable timestamp (missing or unparseable). */ + readonly hasMs: boolean; + /** The parsed key; meaningless (0) when `hasMs` is false. */ + readonly ms: number; +}; + +/** + * The A2.2 sort key, read once per item: the first comment's `createdAt` for a + * thread, the comment's own `createdAt` for a conversation comment. `null` + * means missing or unparseable; those items sort after every timed item. + */ +function timestampMsFor(item: DiscussionListItem): number | null { + const raw = + item.kind === 'thread' ? (item.thread.comments[0]?.createdAt ?? null) : item.comment.createdAt; + if (raw == null || raw === '') { + return null; + } + const ms = parseTimestamp(raw).getTime(); + return Number.isFinite(ms) ? ms : null; +} + +function decorateForMerge(item: DiscussionListItem): DecoratedDiscussionListItem { + const ms = timestampMsFor(item); + return { item, hasMs: ms !== null, ms: ms ?? 0 }; +} + +/** + * Same total order as `compareDiscussionListItems`, but the timestamp key is + * precomputed. Exact ties (or two missing timestamps) delegate to the shared + * comparator for the kind/identity tie-break. + */ +function compareDecoratedItems( + a: DecoratedDiscussionListItem, + b: DecoratedDiscussionListItem +): number { + if (a.hasMs && b.hasMs && a.ms !== b.ms) { + return a.ms - b.ms; + } + if (a.hasMs !== b.hasMs) { + return a.hasMs ? -1 : 1; + } + return compareDiscussionListItems(a.item, b.item); +} + +/** + * Merge review threads and conversation comments into one ascending list, + * parsing each item's timestamp once per merge instead of once per comparison. + * Order-equivalent to `mergeDiscussionListItems` (R4: a "Load more" page can + * insert rows mid-list; the whole loaded set is re-sorted). + */ +export function mergeDiscussionListItemsBySortKey( + threads: readonly ReviewThread[], + conversation: readonly ConversationComment[] +): readonly DiscussionListItem[] { + const decorated: DecoratedDiscussionListItem[] = [ + ...threads.map(thread => decorateForMerge({ kind: 'thread', thread })), + ...conversation.map(comment => decorateForMerge({ kind: 'comment', comment })), + ]; + decorated.sort(compareDecoratedItems); + return decorated.map(entry => entry.item); +} diff --git a/apps/mobile/src/lib/pr-review/use-provider-inbox.mounted.test.tsx b/apps/mobile/src/lib/pr-review/use-provider-inbox.mounted.test.tsx index 64f776c6fca..d9c1f0c3e2f 100644 --- a/apps/mobile/src/lib/pr-review/use-provider-inbox.mounted.test.tsx +++ b/apps/mobile/src/lib/pr-review/use-provider-inbox.mounted.test.tsx @@ -1,5 +1,5 @@ -import './use-provider-inbox.test-helpers'; -import { act, createElement } from 'react'; +import { parseTimestampSpy } from './use-provider-inbox.test-helpers'; +import { act, createElement, useState } from 'react'; import { beforeEach, describe, expect, it, onTestFinished, vi } from 'vitest'; import { PrReviewInboxList } from '@/components/pr-review/pr-review-inbox-list'; @@ -78,7 +78,12 @@ beforeEach(() => { async function mountInbox(enabled = true) { let inbox: ReturnType | undefined = undefined; + let bump: (() => void) | undefined = undefined; function Harness() { + const [, setTick] = useState(0); + bump = () => { + setTick(tick => tick + 1); + }; inbox = useProviderInbox(enabled); return null; } @@ -92,6 +97,13 @@ async function mountInbox(enabled = true) { } return inbox; }, + // Harness-local state bump: re-renders the hook with unchanged query data. + bump: () => { + if (!bump) { + throw new Error('Inbox did not mount'); + } + bump(); + }, }; } @@ -285,3 +297,22 @@ describe('revoked GitHub inbox recovery', () => { } ); }); + +it('reuses the merged items array across a re-render with unchanged query data', async () => { + mocks.authorization.mockResolvedValue({ connected: true }); + const mounted = await mountInbox(); + await waitFor(() => mounted.inbox().items.length === 2); + // Before the fix the merge ran in the render body, so each render produced a + // fresh array and re-parsed every row. The memo must reuse both. + const itemsBefore = mounted.inbox().items; + const parsesBefore = parseTimestampSpy.mock.calls.length; + act(() => { + mounted.bump(); + }); + // eslint-disable-next-line no-console -- the PR proof quotes this measured parse count + console.log( + `[inbox] parses load=${parsesBefore} rerender=${parseTimestampSpy.mock.calls.length}` + ); + expect(mounted.inbox().items).toBe(itemsBefore); + expect(parseTimestampSpy.mock.calls.length).toBe(parsesBefore); +}); diff --git a/apps/mobile/src/lib/pr-review/use-provider-inbox.test-helpers.ts b/apps/mobile/src/lib/pr-review/use-provider-inbox.test-helpers.ts index 126a9fa0749..0b8b035b51f 100644 --- a/apps/mobile/src/lib/pr-review/use-provider-inbox.test-helpers.ts +++ b/apps/mobile/src/lib/pr-review/use-provider-inbox.test-helpers.ts @@ -1,6 +1,25 @@ import { createElement, type ReactNode } from 'react'; import { vi } from 'vitest'; +import type * as UtilsModule from '@/lib/utils'; + +/** + * `parseTimestamp` spy for tests that count parses. This helper registers the + * partial `@/lib/utils` mock (`utilsMockFactory`) for every file that imports + * it, so a test asserts on `parseTimestampSpy`; the factory delegates each call + * to the real export and leaves every other `@/lib/utils` export untouched. + */ +export const parseTimestampSpy = vi.fn(); + +async function utilsMockFactory( + importOriginal: () => Promise +): Promise> { + const actual = await importOriginal(); + parseTimestampSpy.mockImplementation(actual.parseTimestamp); + return { ...actual, parseTimestamp: parseTimestampSpy }; +} +vi.mock('@/lib/utils', utilsMockFactory); + // Native hosts only: the inbox, authorization queries, and reconnect mutation // remain real in the mounted recovery tests. vi.mock('react-native', () => ({ View: 'View', Pressable: 'Pressable' })); diff --git a/apps/mobile/src/lib/pr-review/use-provider-inbox.test.ts b/apps/mobile/src/lib/pr-review/use-provider-inbox.test.ts index 82e92f18314..1b962b369bf 100644 --- a/apps/mobile/src/lib/pr-review/use-provider-inbox.test.ts +++ b/apps/mobile/src/lib/pr-review/use-provider-inbox.test.ts @@ -1,3 +1,6 @@ +// Imported first: the helper registers the partial `@/lib/utils` mock (a +// `parseTimestamp` spy delegating to the real export) before the hook loads. +import { parseTimestampSpy } from './use-provider-inbox.test-helpers'; import { describe, expect, it, vi } from 'vitest'; import { classifyPrReviewQueryState } from './classify-pr-review-query-state'; @@ -124,6 +127,19 @@ describe('inbox rows', () => { }); describe('mergeProviderInboxSources', () => { + it('parses each row timestamp exactly once per merge', () => { + // The comparator used to call `parseTimestamp` on BOTH operands of every + // comparison, so a three-provider merge re-parsed its timestamps O(n log n) + // times. The merge decorates once, so the count equals the row count. + const spy = vi.mocked(parseTimestampSpy).mockClear(); + mergeProviderInboxSources([ + source({ platform: 'github', rows: githubRows }), + source({ platform: 'gitlab', rows: gitlabRows }), + source({ platform: 'bitbucket', rows: bitbucketRows }), + ]); + expect(spy).toHaveBeenCalledTimes(3); + }); + it('merges every enabled provider newest-first', () => { const merged = mergeProviderInboxSources([ source({ platform: 'github', rows: githubRows }), diff --git a/apps/mobile/src/lib/pr-review/use-provider-inbox.ts b/apps/mobile/src/lib/pr-review/use-provider-inbox.ts index fbce265cb7f..7d001b335b0 100644 --- a/apps/mobile/src/lib/pr-review/use-provider-inbox.ts +++ b/apps/mobile/src/lib/pr-review/use-provider-inbox.ts @@ -134,10 +134,15 @@ export function mergeProviderInboxSources( sources: readonly ProviderInboxSource[] ): MergedProviderInbox { const active = sources.filter(source => source.enabled); - const items = active - .flatMap(source => source.rows) - // eslint-disable-next-line unicorn/no-array-sort -- Hermes does not implement Array.prototype.toSorted; flatMap already copies so nothing shared is mutated - .sort((left, right) => updatedAtMs(right) - updatedAtMs(left)); + // Decorate once, then sort on the precomputed milliseconds: a plain + // comparator would re-parse BOTH timestamps on every comparison, turning a + // few hundred rows into thousands of `parseTimestamp` calls per merge. + const decorated = active.flatMap(source => + source.rows.map(row => ({ row, ms: updatedAtMs(row) })) + ); + // eslint-disable-next-line unicorn/no-array-sort -- Hermes does not implement Array.prototype.toSorted; flatMap already copies so nothing shared is mutated + decorated.sort((left, right) => right.ms - left.ms); + const items = decorated.map(entry => entry.row); const failed = active.filter(source => source.error !== null && source.error !== undefined); const firstPageFailures = failed.filter(source => !source.hasLoadedPages); const allFailed = active.length > 0 && firstPageFailures.length === active.length; @@ -300,7 +305,41 @@ export function useProviderInbox(enabled: boolean) { hasNextPage: bitbucket.hasNextPage, isFetchingNextPage: bitbucket.isFetchingNextPage, }; - const merged = mergeProviderInboxSources([githubSource, gitlabSource, bitbucketSource]); + // Memoized on every value the merge reads, so the merge (and its sort) only + // re-runs when an input actually moved. Keying on the source values rather + // than the per-render `*Source` objects keeps the merge off the render path + // while `retryFailedPages` below still reads the fresh objects it needs. + const merged = useMemo( + () => mergeProviderInboxSources([githubSource, gitlabSource, bitbucketSource]), + // eslint-disable-next-line react/exhaustive-deps -- the deps below are exactly the values the three sources read, so the freshly built *Source objects add nothing to compare + [ + githubRows, + gitlabRows, + bitbucketRows, + enabled, + githubEnabled, + gitlabEnabled, + bitbucketEnabled, + githubAuthorization.isPending, + githubAuthorization.isError, + githubAuthorization.error, + github.query.isPending, + github.query.data, + github.query.error, + github.query.hasNextPage, + github.query.isFetchingNextPage, + gitlab.isPending, + gitlab.error, + gitlab.hasNextPage, + gitlab.isFetchingNextPage, + gitlabPages, + bitbucket.isPending, + bitbucket.error, + bitbucket.hasNextPage, + bitbucket.isFetchingNextPage, + bitbucketPages, + ] + ); // Every source advances on ITS OWN cursor; nothing is shared or merged. const fetchNextPage = useCallback(() => {