From a0c238390b3829aeb915a3c357be7314d7d645d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Sun, 20 Sep 2026 20:18:27 +0200 Subject: [PATCH] fix(mobile): show an explained empty state in the PR discussion tab https://github.com/Kilo-Org/cloud/pull/6375 --- ...pr-review-discussion-list.mounted.test.tsx | 200 +++++++++++++++--- .../pr-review-discussion-list.test.tsx | 2 + .../discussion/pr-review-discussion-list.tsx | 25 +++ .../pr-review-discussion-tab.test.tsx | 25 +++ .../pr-review/pr-review-discussion-tab.tsx | 5 +- .../src/lib/auth/passkey-client.test.ts | 12 ++ .../lib/pr-review/provider-pr-queries.test.ts | 39 ++++ .../src/lib/pr-review/provider-pr-queries.ts | 12 +- 8 files changed, 292 insertions(+), 28 deletions(-) diff --git a/apps/mobile/src/components/pr-review/discussion/pr-review-discussion-list.mounted.test.tsx b/apps/mobile/src/components/pr-review/discussion/pr-review-discussion-list.mounted.test.tsx index 3573ada9acb..17bfd1f5b19 100644 --- a/apps/mobile/src/components/pr-review/discussion/pr-review-discussion-list.mounted.test.tsx +++ b/apps/mobile/src/components/pr-review/discussion/pr-review-discussion-list.mounted.test.tsx @@ -1,14 +1,22 @@ -import { createElement, Fragment, type ReactNode } from 'react'; +import { type ComponentProps, createElement, Fragment, type ReactNode } from 'react'; import { act, TestRenderer } from '@/test/renderer'; import { beforeEach, describe, expect, it, vi } from 'vitest'; -import { type DiscussionListItem } from '@/lib/pr-review/discussion/review-discussion-types'; +import { + type ConversationComment, + type DiscussionListItem, + type ReviewThread, +} from '@/lib/pr-review/discussion/review-discussion-types'; import { ProviderPrScopeProvider } from '@/lib/pr-review/provider-pr-ref'; import { PrReviewDiscussionList } from './pr-review-discussion-list'; type TaggedOptions = { tag: string; input?: unknown }; -const observed = vi.hoisted(() => ({ options: [] as TaggedOptions[] })); +const observed = vi.hoisted(() => ({ + options: [] as TaggedOptions[], + blockedLogins: [] as string[], + mutedLogins: [] as string[], +})); // Records every query the list mounts and answers the overview with a viewer // login, so a test can assert BOTH which namespace was asked and what the @@ -20,7 +28,7 @@ vi.mock('@tanstack/react-query', () => ({ useQuery: (options: TaggedOptions) => { observed.options.push(options); if (options.tag === 'moderation') { - return { data: { blockedLogins: [], mutedLogins: [] } }; + return { data: { blockedLogins: observed.blockedLogins, mutedLogins: observed.mutedLogins } }; } return { data: { repo: { viewerLogin: 'octocat' } } }; }, @@ -46,16 +54,23 @@ vi.mock('@shopify/flash-list', () => ({ FlashList: ({ data, renderItem, + ListEmptyComponent, + ListFooterComponent, }: { data: readonly unknown[]; renderItem: (args: { item: unknown; index: number }) => ReactNode; + ListEmptyComponent?: ReactNode; + ListFooterComponent?: ReactNode; }) => createElement( Fragment, null, - data.map((item, index) => - createElement(Fragment, { key: index }, renderItem({ item, index })) - ) + data.length === 0 + ? ListEmptyComponent + : data.map((item, index) => + createElement(Fragment, { key: index }, renderItem({ item, index })) + ), + ListFooterComponent ), })); vi.mock('@/components/pr-review/discussion/comment-row', () => ({ CommentRow: 'CommentRow' })); @@ -64,30 +79,54 @@ vi.mock('@/components/pr-review/discussion/discussion-thread', () => ({ })); vi.mock('@/components/ui/button', () => ({ Button: 'Button' })); vi.mock('@/components/ui/text', () => ({ Text: 'Text' })); +vi.mock('@/components/empty-state', () => ({ EmptyState: 'EmptyState' })); +vi.mock('@/components/ui/icons', () => ({ MessageSquarePlus: 'MessageSquarePlus' })); vi.mock('@/lib/screen-insets', () => ({ useDetailScreenBottomPadding: () => 0 })); function noop(): void { - // The viewer query is what this file asserts; the list callbacks are inert. + // Callbacks not exercised by a test are inert. +} + +function makeComment(id: number, login: string | null = 'octocat'): ConversationComment { + return { + commentId: id, + nodeId: `c${id}`, + author: login === null ? null : { login, avatarUrl: null }, + bodyMarkdown: 'hello', + createdAt: '2026-01-01T00:00:00Z', + reactions: [], + }; } -const listItems: readonly DiscussionListItem[] = [ - { - kind: 'comment', - comment: { - commentId: 1, - nodeId: 'c1', - author: { login: 'octocat', avatarUrl: null }, - bodyMarkdown: 'hello', - createdAt: '2026-01-01T00:00:00Z', - reactions: [], +function makeThread(comments: ReviewThread['comments']): DiscussionListItem { + return { + kind: 'thread', + thread: { + 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, }, - }, -]; + }; +} + +const listItems: readonly DiscussionListItem[] = [{ kind: 'comment', comment: makeComment(1) }]; -function mountList(scope?: { - ref: { platform: 'gitlab'; projectPath: string; mrIid: number }; - organizationId: string | null; -}) { +function mountList( + scope?: { + ref: { platform: 'gitlab'; projectPath: string; mrIid: number }; + organizationId: string | null; + }, + overrides: Partial> = {} +) { const list = ( ); const created: { current: TestRenderer.ReactTestRenderer | undefined } = { current: undefined }; @@ -119,11 +159,120 @@ function mountList(scope?: { return renderer; } -describe('PrReviewDiscussionList viewer query', () => { +describe('PrReviewDiscussionList visible content', () => { beforeEach(() => { observed.options = []; + observed.blockedLogins = []; + observed.mutedLogins = []; }); + it.each(['blockedLogins', 'mutedLogins'] as const)( + 'explains an empty visible discussion when every author is in %s', + hiddenUsers => { + observed[hiddenUsers] = ['OCTOCAT']; + const renderer = mountList(undefined, { + listItems: Array.from({ length: 5 }, (_, index) => ({ + kind: 'comment', + comment: makeComment(index + 1), + })), + }); + + expect(renderer.root.findAll(node => String(node.type) === 'CommentRow')).toHaveLength(0); + const emptyStates = renderer.root.findAll(node => String(node.type) === 'EmptyState'); + expect(emptyStates).toHaveLength(1); + expect(emptyStates[0]?.props.title).toBe('prReview.discussion.noDiscussion'); + expect(emptyStates[0]?.props.description).toBe('prReview.discussion.noDiscussionDescription'); + // The FlashList owns scrolling; do not nest the centered state's ScrollView. + expect(emptyStates[0]?.props.placement).toBe('top'); + // The same empty state names a merge request under a GitLab scope. + const gitlab = mountList( + { + ref: { platform: 'gitlab', projectPath: 'group/sub/repo', mrIid: 12 }, + organizationId: null, + }, + { listItems: [{ kind: 'comment', comment: makeComment(1) }] } + ); + expect(gitlab.root.findByType('EmptyState').props.description).toBe( + 'prReview.terms.noDiscussionDescription' + ); + gitlab.unmount(); + + renderer.unmount(); + } + ); + + it.each([{ comments: [] }, { comments: [makeComment(1)] }])( + 'explains threads with no visible comments ($comments)', + ({ comments }) => { + observed.blockedLogins = ['octocat']; + const renderer = mountList(undefined, { listItems: [makeThread(comments)] }); + expect(renderer.root.findAllByType('DiscussionThread')).toHaveLength(0); + expect(renderer.root.findAllByType('EmptyState')).toHaveLength(1); + renderer.unmount(); + } + ); + + it('keeps visible replies and deleted-author comments without an empty state', () => { + observed.mutedLogins = ['octocat']; + const visibleReply = makeComment(2, 'alice'); + const renderer = mountList(undefined, { + listItems: [ + ...listItems, + { kind: 'comment', comment: makeComment(3, null) }, + makeThread([makeComment(1), visibleReply]), + ], + }); + expect(renderer.root.findAllByType('CommentRow')).toHaveLength(1); + expect(renderer.root.findByType('DiscussionThread').props.thread).toMatchObject({ + comments: [visibleReply], + }); + expect(renderer.root.findAllByType('EmptyState')).toHaveLength(0); + renderer.unmount(); + }); + + it.each(['load-more', 'loading', 'retry'] as const)( + 'keeps the %s footer usable when all loaded comments are hidden', + state => { + observed.mutedLogins = ['octocat']; + const onLoadMore = vi.fn<() => void>(); + const onRetryLoadMore = vi.fn<() => void>(); + const renderer = mountList(undefined, { + hasNextPage: true, + isFetchingNextPage: state === 'loading', + laterPageError: state === 'retry', + onLoadMore, + onRetryLoadMore, + }); + expect(renderer.root.findAllByType('EmptyState')).toHaveLength(1); + const button = renderer.root.findByType('Button'); + if (state === 'loading') { + expect(button.props.loading).toBe(true); + } else { + act(() => { + (button.props.onPress as () => void)(); + }); + expect(state === 'retry' ? onRetryLoadMore : onLoadMore).toHaveBeenCalledOnce(); + } + // A later page can reveal a visible comment without remounting the list surface. + const props = renderer.root.findByType(PrReviewDiscussionList).props as ComponentProps< + typeof PrReviewDiscussionList + >; + act(() => { + renderer.update( + + ); + }); + expect(renderer.root.findAllByType('EmptyState')).toHaveLength(0); + expect(renderer.root.findAllByType('CommentRow')).toHaveLength(1); + renderer.unmount(); + } + ); + it('reads the viewer login from the GitHub procedure on a GitHub scope', () => { const renderer = mountList(); @@ -135,6 +284,7 @@ describe('PrReviewDiscussionList viewer query', () => { expect( renderer.root.findAll(node => String(node.type) === 'CommentRow')[0]?.props.viewerLogin ).toBe('octocat'); + expect(renderer.root.findAllByType('EmptyState')).toHaveLength(0); act(() => { renderer.unmount(); diff --git a/apps/mobile/src/components/pr-review/discussion/pr-review-discussion-list.test.tsx b/apps/mobile/src/components/pr-review/discussion/pr-review-discussion-list.test.tsx index 646bfbdc35f..f2df02d8fc7 100644 --- a/apps/mobile/src/components/pr-review/discussion/pr-review-discussion-list.test.tsx +++ b/apps/mobile/src/components/pr-review/discussion/pr-review-discussion-list.test.tsx @@ -30,6 +30,8 @@ vi.mock('@/lib/trpc', () => ({ })); vi.mock('@/components/ui/button', () => ({ Button: 'Button' })); vi.mock('@/components/ui/text', () => ({ Text: 'Text' })); +vi.mock('@/components/empty-state', () => ({ EmptyState: 'EmptyState' })); +vi.mock('@/components/ui/icons', () => ({ MessageSquarePlus: 'MessageSquarePlus' })); vi.mock('@/components/pr-review/discussion/comment-row', () => ({ CommentRow: 'CommentRow' })); vi.mock('@/components/pr-review/discussion/discussion-thread', () => ({ DiscussionThread: 'DiscussionThread', diff --git a/apps/mobile/src/components/pr-review/discussion/pr-review-discussion-list.tsx b/apps/mobile/src/components/pr-review/discussion/pr-review-discussion-list.tsx index 6e789e2436c..b3e14bf1ad5 100644 --- a/apps/mobile/src/components/pr-review/discussion/pr-review-discussion-list.tsx +++ b/apps/mobile/src/components/pr-review/discussion/pr-review-discussion-list.tsx @@ -9,9 +9,11 @@ import { useTranslation } from 'react-i18next'; import { View, type ViewStyle } from 'react-native'; import { useSafeAreaInsets } from 'react-native-safe-area-context'; +import { EmptyState } from '@/components/empty-state'; import { CommentRow } from '@/components/pr-review/discussion/comment-row'; import { DiscussionThread } from '@/components/pr-review/discussion/discussion-thread'; import { Button } from '@/components/ui/button'; +import { MessageSquarePlus } from '@/components/ui/icons'; import { Text } from '@/components/ui/text'; import { type DiscussionListItem, @@ -19,6 +21,7 @@ import { } from '@/lib/pr-review/discussion/review-discussion-types'; import { expandedForThread } from '@/lib/pr-review/discussion/thread-expansion'; import { useProviderPrQueries } from '@/lib/pr-review/provider-pr-queries'; +import { useProviderPrScope } from '@/lib/pr-review/provider-pr-ref'; import { useDetailScreenBottomPadding } from '@/lib/screen-insets'; import { useTRPC } from '@/lib/trpc'; @@ -74,7 +77,12 @@ export function PrReviewDiscussionList({ onReplyInputFocus, onViewportLayout, }: Readonly) { + const { t } = useTranslation(); const trpc = useTRPC(); + // Provider noun for the empty message below (GitLab calls it a merge + // request); the same scope the tab reads for its own copy. + const { ref } = useProviderPrScope({ owner, repo, number }); + const isMergeRequest = ref.platform === 'gitlab'; // Account-local hidden users (blocked + muted GitHub logins) filter rows. const hiddenUsers = useQuery(trpc.moderation.listHiddenUsers.queryOptions()); // Viewer login for self-target gating on the comment overflow menu. The @@ -190,6 +198,23 @@ export function PrReviewDiscussionList({ onLayout={event => { onViewportLayout?.(event.nativeEvent.layout.height); }} + ListEmptyComponent={ + // Every loaded row can be hidden (blocked/muted authors) while the + // tab still counts the discussion as content. Without this the list + // renders an empty body under the pinned Comment bar: no rows, no + // loading, no message (spot check e7). The copy is the discussion's + // existing empty copy — new copy belongs to the translation slice. + + } ListFooterComponent={ { expect(renderer.root.findAll(node => String(node.type) === 'QueryError')).toHaveLength(0); }); + it.each(['load-more', 'loading', 'retry'] as const)( + 'keeps the %s footer reachable when normalization leaves no discussion rows', + state => { + discussionState.query.hasNextPage = state !== 'retry'; + discussionState.query.isFetchingNextPage = state === 'loading'; + discussionState.laterPageError = state === 'retry'; + const renderer = mountTab(); + const list = renderer.root.find(node => String(node.type) === 'PrReviewDiscussionList'); + expect(list.props.listItems).toEqual([]); + expect(list.props.hasNextPage).toBe(state !== 'retry'); + expect(list.props.isFetchingNextPage).toBe(state === 'loading'); + expect(list.props.laterPageError).toBe(state === 'retry'); + expectCtaPresence(renderer, true); + if (state !== 'loading') { + act(() => { + (list.props[state === 'retry' ? 'onRetryLoadMore' : 'onLoadMore'] as () => void)(); + }); + expect( + state === 'retry' ? discussionState.query.refetch : discussionState.query.fetchNextPage + ).toHaveBeenCalledOnce(); + } + renderer.unmount(); + } + ); + it('keeps permission denial ahead of retained comments', () => { discussionState.conversation = [{ nodeId: 'c1', createdAt: null }]; discussionState.firstPageErrorState = { kind: 'permission' }; 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 5c09b32aefa..abdc27ff9a0 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 @@ -368,8 +368,9 @@ export function PrReviewDiscussionTab({ ); } - // ── Empty (neither threads nor conversation comments) ────────────── - if (view.kind === 'empty') { + // An empty normalized page can still have more discussion to load. Keep + // the list's empty message and pagination footer reachable in that case. + if (view.kind === 'empty' && !query.hasNextPage && !query.isFetchingNextPage && !laterPageError) { return withCommentCta( { expect(classifyPasskeyError({ message: 'nocredentials' })).toBe('no-passkey'); }); + it.each([ + [{ name: 'USERCANCELLEDEXCEPTION' }, 'cancelled', 'login.passkeyCancelled'], + [{ code: 'NoCrEdEnTiAlS' }, 'no-passkey', 'login.passkeyNotFound'], + [{ message: 'NOTCONFIGURED' }, 'unsupported', 'login.passkeyUnsupported'], + [{ code: 'NoTsUpPoRtEd' }, 'unsupported', 'login.passkeyUnsupported'], + [{ name: 'UNKNOWNERROR' }, 'failed', 'login.passkeyFailed'], + ])('classifies %j without re-casing the catalog key', (nativeError, failure, key) => { + const classified = classifyPasskeyError(nativeError); + expect(classified).toBe(failure); + expect(passkeyFailureKey(classified)).toBe(key); + }); + it.each([ [{ name: 'USERCANCELLEDEXCEPTION' }, 'login.passkeyCancelled'], [{ message: 'NOCREDENTIALS' }, 'login.passkeyNotFound'], diff --git a/apps/mobile/src/lib/pr-review/provider-pr-queries.test.ts b/apps/mobile/src/lib/pr-review/provider-pr-queries.test.ts index 8b61ee2c754..055983219de 100644 --- a/apps/mobile/src/lib/pr-review/provider-pr-queries.test.ts +++ b/apps/mobile/src/lib/pr-review/provider-pr-queries.test.ts @@ -13,6 +13,7 @@ import { normalizeProviderThreadsPage, normalizePrThreadsPages, providerCapabilitiesIdentity, + type PrThreadsPageModel, } from './provider-pr-queries'; import { githubPrRef, isProviderScopeReady, type ProviderPrScope } from './provider-pr-ref'; @@ -368,6 +369,44 @@ describe('normalizeProviderThreadsPage', () => { expect(normalizePrThreadsPages('gitlab', undefined)).toEqual([]); }); + it('drops a GitHub discussion whose comment list is empty', () => { + // GitHub can hand back a review thread with no comments, the same shape the + // GitLab arm drops above. It is not discussion content: counted as content + // it keeps the tab out of its empty state, and the list's visible-comment + // filter then drops it, so the tab renders a blank body with no comments, + // no empty state and no error (spot check e7). + const githubThread: PrThreadsPageModel['threads'][number] = { + threadId: 't-1', + isResolved: false, + isOutdated: false, + subjectType: 'LINE', + path: 'src/a.ts', + line: 12, + startLine: null, + originalLine: null, + originalStartLine: null, + diffSide: 'RIGHT', + diffHunk: null, + comments: [ + { + commentId: 1, + nodeId: 'c1', + author: { login: 'ada', avatarUrl: null }, + bodyMarkdown: 'nit', + createdAt: '2026-01-01T00:00:00Z', + reactions: [], + }, + ], + }; + const page = { + threads: [githubThread, { ...githubThread, threadId: 't-empty', comments: [] }], + conversation: [], + nextCursor: null, + }; + const [normalized] = normalizePrThreadsPages('github', [page]); + expect(normalized?.threads.map(entry => entry.threadId)).toEqual(['t-1']); + }); + it('drops a discussion whose notes are all system events', () => { // GitLab reports every MR event (pushes, assignments) as a discussion, // and the read layer strips the system notes — what remains carries no diff --git a/apps/mobile/src/lib/pr-review/provider-pr-queries.ts b/apps/mobile/src/lib/pr-review/provider-pr-queries.ts index fe66f2987e6..de7f316305d 100644 --- a/apps/mobile/src/lib/pr-review/provider-pr-queries.ts +++ b/apps/mobile/src/lib/pr-review/provider-pr-queries.ts @@ -233,7 +233,17 @@ export function normalizePrThreadsPages( ): PrThreadsPageModel[] { const loaded = pages ?? []; if (platform === 'github') { - return [...loaded]; + // A GitHub review thread can arrive with an empty comment list — the + // shape the provider arm below also drops. A thread with no comments is + // not discussion content: counted as content it keeps the tab out of its + // empty state, and the list's visible-comment filter then drops it, so + // the tab renders a blank body with no comments, no empty state and no + // error (spot check e7). Drop it here, where the shape is read, so the + // tab's empty check sees the truth. + return loaded.map(page => ({ + ...page, + threads: page.threads.filter(thread => thread.comments.length > 0), + })); } return asGithubOptions(loaded).map(page => normalizeProviderThreadsPage(page)