Skip to content

Commit bd4668e

Browse files
authored
fix(mobile): open the reviewed pull request without a GitHub connection (#9305)
* fix(mobile): open the reviewed pull request without a GitHub connection A personal Code Reviewer review can exist for a user who never authorized the GitHub app, and the review detail sent every github.com pull request to the in-app reader. That route is gated on the user's own GitHub authorization, so the button ended on the Connect empty state instead of the pull request. Open the pull request in the app only when the authorization read reports connected; otherwise (not connected, still loading, or failed) open the review's pr_url in the browser, as organization reviews already do. The connect gate itself is unchanged, so the comment, review and merge actions still require the authorization. * fix(mobile): tidy the code review detail screen The review detail painted its status twice: the conclusion line above the findings and a Status row in the Gate card. The Gate card now carries the check run and the threshold only. The pull request is the screen's primary action, so it takes the filled primary button style; the session transcript entry stays secondary. The byline under the pull request title repeated the repository the header eyebrow already names. It now reads the number and the author, and every catalog drops the repository placeholder with it. * fix(mobile): decide the review pull-request reader from its URL The connection read that the Open pull request button consults was enabled from the review row's platform column, while the button's in-app decision already followed the provider parsed from the pull-request URL. A row whose column disagreed with its URL therefore read the authorization for the wrong provider: a connected user whose row said another platform still went to the browser. Both now use the one URL-derived destination, computed once on the screen, so the button and its read can never disagree. A mounted test covers the mismatched row. * fix(mobile): restyle the review detail's empty result and actions The empty Findings section now sits in the same rounded card as the Gate and Details bodies, so an ended review with nothing to report reads as a filled section instead of a caption. Session transcript moves from the top of the content into the action group, after the primary Open pull request button, so the review title leads the screen. The mounted suite asserts both placements. * test(mobile): require each reference label before comparing placements The transcript placement test compared indexes that are -1 for absent copy, and every ordering assertion holds against -1. Assert the title, Details and Open pull request labels are present first.
1 parent 274f33d commit bd4668e

92 files changed

Lines changed: 296 additions & 124 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎apps/mobile/src/components/code-reviewer/review-detail-screen.mounted.test.tsx‎

Lines changed: 143 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,10 @@ const reviewConfig = vi.hoisted(() => ({
7373
calls: [] as { scope: string; platform: string; enabled: boolean | undefined }[],
7474
}));
7575
const routerMock = vi.hoisted(() => ({ push: vi.fn() }));
76+
const githubAuthorization = vi.hoisted(() => ({
77+
data: undefined as { connected: boolean } | undefined,
78+
enabled: [] as (boolean | undefined)[],
79+
}));
7680
const spectatorStream = vi.hoisted(() => ({
7781
createReviewSpectatorStream: vi.fn(),
7882
}));
@@ -175,6 +179,11 @@ vi.mock('@/lib/utils', () => ({
175179

176180
vi.mock('@/lib/trpc', () => ({
177181
useTRPC: () => ({
182+
githubApps: {
183+
getUserAuthorization: {
184+
queryOptions: () => ({ queryKey: ['githubApps.getUserAuthorization'] }),
185+
},
186+
},
178187
codeReviews: {
179188
getReviewStreamInfo: {
180189
queryOptions: () => ({ queryKey: ['codeReviews.getReviewStreamInfo'] }),
@@ -188,6 +197,10 @@ vi.mock('@/lib/trpc', () => ({
188197
vi.mock('@tanstack/react-query', () => ({
189198
useQuery: (options: { queryKey?: unknown[]; enabled?: boolean; refetchInterval?: unknown }) => {
190199
const key = options.queryKey?.[0];
200+
if (key === 'githubApps.getUserAuthorization') {
201+
githubAuthorization.enabled.push(options.enabled);
202+
return { data: githubAuthorization.data };
203+
}
191204
if (key === 'codeReviews.getReviewStreamInfo') {
192205
return spectatorQueries.streamInfo;
193206
}
@@ -313,6 +326,8 @@ beforeEach(() => {
313326
reviewConfig.query = { isPending: false, data: undefined };
314327
reviewConfig.calls = [];
315328
routerMock.push.mockClear();
329+
githubAuthorization.data = undefined;
330+
githubAuthorization.enabled = [];
316331
vi.mocked(openExternalUrl).mockClear();
317332
nativePlatform.OS = 'ios';
318333
sessionListRenders.list = [];
@@ -379,6 +394,25 @@ describe('ReviewDetailScreen outcome-first order', () => {
379394
expect(texts).toContain('$123.00');
380395
});
381396

397+
it('states the review status once, above the findings', () => {
398+
detail.data = {
399+
success: true,
400+
review: makeReview({ status: 'completed', completed_at: '2024-01-01T00:05:00.000Z' }),
401+
tokenUsage: { input: 0, output: 0 },
402+
};
403+
404+
const texts = renderScreen();
405+
const findingsIndex = texts.indexOf('Findings');
406+
const detailsIndex = texts.indexOf('Details');
407+
expect(findingsIndex).toBeGreaterThanOrEqual(0);
408+
expect(detailsIndex).toBeGreaterThan(findingsIndex);
409+
410+
// The conclusion above the findings owns the status word; the council and
411+
// gate cards never repeat it.
412+
expect(texts.slice(0, findingsIndex)).toContain('Completed');
413+
expect(texts.slice(findingsIndex, detailsIndex)).not.toContain('Completed');
414+
});
415+
382416
it.each([
383417
['unrecognized internal failure', 'codeReviewer.reviewDetail.reviewFailed'],
384418
[
@@ -524,8 +558,9 @@ describe('ReviewDetailScreen details metadata', () => {
524558
// screen passes.
525559
expect(collectText(completionRows[0]?.children ?? [])).toEqual([completedLabel, 'now']);
526560

527-
// The row belongs to the Details card, not the Gate's status row, which
528-
// also shows "Completed" but as the value under the "Status" label.
561+
// The row belongs to the Details card. The Gate card carries the check run
562+
// and the threshold only, so the review status paints once, as the
563+
// conclusion above the findings.
529564
const texts = collectText(renderer.toJSON());
530565
const completionRowIndex = texts.findIndex(
531566
(text, index) => text === completedLabel && texts[index + 1] === 'now'
@@ -759,9 +794,94 @@ describe('ReviewDetailScreen pull request destination', () => {
759794
expect(routerMock.push).not.toHaveBeenCalled();
760795
});
761796

762-
it('keeps personal GitHub reviews in app', () => {
797+
it('opens a personal GitHub review in app when the user connection is live', () => {
798+
detail.data = { success: true, review: makeReview(), tokenUsage: { input: 0, output: 0 } };
799+
githubAuthorization.data = { connected: true };
800+
mountScreen();
801+
const button = buttons.rendered.find(
802+
candidate => buttonText(candidate) === i18n.t('common.openPullRequest')
803+
);
804+
expect(button?.onPress).toBeDefined();
805+
act(() => button?.onPress?.());
806+
expect(routerMock.push).toHaveBeenCalledWith('/(app)/pr-review/org/repo/42');
807+
expect(openExternalUrl).not.toHaveBeenCalled();
808+
});
809+
810+
it('opens a personal GitHub review in the browser when the user connection is missing', () => {
811+
const prUrl = 'https://github.com/org/repo/pull/42';
812+
detail.data = {
813+
success: true,
814+
review: makeReview({ pr_url: prUrl }),
815+
tokenUsage: { input: 0, output: 0 },
816+
};
817+
githubAuthorization.data = { connected: false };
818+
mountScreen();
819+
const button = buttons.rendered.find(
820+
candidate => buttonText(candidate) === i18n.t('common.openPullRequest')
821+
);
822+
expect(button?.onPress).toBeDefined();
823+
act(() => button?.onPress?.());
824+
expect(openExternalUrl).toHaveBeenCalledWith(prUrl, { label: i18n.t('common.pullRequest') });
825+
expect(routerMock.push).not.toHaveBeenCalled();
826+
});
827+
828+
it('opens a personal GitHub review in the browser before the connection read answers', () => {
829+
const prUrl = 'https://github.com/org/repo/pull/42';
830+
detail.data = {
831+
success: true,
832+
review: makeReview({ pr_url: prUrl }),
833+
tokenUsage: { input: 0, output: 0 },
834+
};
835+
// No data: the read is pending or failed, neither of which answers
836+
// `connected: true`, so the pull request opens in the browser.
837+
githubAuthorization.data = undefined;
838+
mountScreen();
839+
const button = buttons.rendered.find(
840+
candidate => buttonText(candidate) === i18n.t('common.openPullRequest')
841+
);
842+
act(() => button?.onPress?.());
843+
expect(openExternalUrl).toHaveBeenCalledWith(prUrl, { label: i18n.t('common.pullRequest') });
844+
expect(routerMock.push).not.toHaveBeenCalled();
845+
});
846+
847+
it('reads the GitHub connection only for a personal GitHub review', () => {
763848
detail.data = { success: true, review: makeReview(), tokenUsage: { input: 0, output: 0 } };
764849
mountScreen();
850+
expect(githubAuthorization.enabled).toContain(true);
851+
852+
githubAuthorization.enabled = [];
853+
mountScreen(false, 'org-1');
854+
expect(githubAuthorization.enabled).toContain(false);
855+
856+
githubAuthorization.enabled = [];
857+
detail.data = {
858+
success: true,
859+
review: makeReview({
860+
platform: 'gitlab',
861+
pr_url: 'https://gitlab.com/org/repo/-/merge_requests/42',
862+
}),
863+
tokenUsage: { input: 0, output: 0 },
864+
};
865+
mountScreen();
866+
expect(githubAuthorization.enabled).toContain(false);
867+
});
868+
869+
it('decides on the URL provider when the row column disagrees', () => {
870+
// The row says gitlab but its URL is a github.com pull request. The button
871+
// and the connection read both follow the URL, so a connected user still
872+
// reads in the app.
873+
detail.data = {
874+
success: true,
875+
review: makeReview({
876+
platform: 'gitlab',
877+
pr_url: 'https://github.com/org/repo/pull/42',
878+
}),
879+
tokenUsage: { input: 0, output: 0 },
880+
};
881+
githubAuthorization.data = { connected: true };
882+
mountScreen();
883+
expect(githubAuthorization.enabled).toContain(true);
884+
765885
const button = buttons.rendered.find(
766886
candidate => buttonText(candidate) === i18n.t('common.openPullRequest')
767887
);
@@ -940,6 +1060,26 @@ describe('ReviewDetailScreen transcript sheet', () => {
9401060
expect(spectatorStream.createReviewSpectatorStream).not.toHaveBeenCalled();
9411061
});
9421062

1063+
it('follows the review content and the primary action instead of leading the screen', () => {
1064+
// The transcript is a secondary way to read the review: the title and the
1065+
// outcome lead, and the entry sits among the actions after Open pull request.
1066+
const renderer = mountScreen();
1067+
const texts = collectText(renderer.toJSON());
1068+
const transcriptIndex = texts.indexOf('Session transcript');
1069+
const titleIndex = texts.indexOf('Fix login');
1070+
const detailsIndex = texts.indexOf('Details');
1071+
const primaryIndex = texts.indexOf('Open pull request');
1072+
1073+
// A missing label is `-1`, which every ordering check below would satisfy,
1074+
// so each reference is asserted present before the order is compared.
1075+
expect(titleIndex).toBeGreaterThanOrEqual(0);
1076+
expect(detailsIndex).toBeGreaterThanOrEqual(0);
1077+
expect(primaryIndex).toBeGreaterThanOrEqual(0);
1078+
expect(transcriptIndex).toBeGreaterThan(titleIndex);
1079+
expect(transcriptIndex).toBeGreaterThan(detailsIndex);
1080+
expect(transcriptIndex).toBeGreaterThan(primaryIndex);
1081+
});
1082+
9431083
it.each(['ios', 'android'])(
9441084
'opens and closes the native transcript sheet on %s',
9451085
async platform => {

‎apps/mobile/src/components/code-reviewer/review-detail-screen.tsx‎

Lines changed: 57 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,8 @@
33
import * as Haptics from 'expo-haptics';
44
import { notificationHaptic } from '@/lib/haptics';
55
import { type Href } from 'expo-router';
6-
import { useRef, useState } from 'react';
6+
import { useQuery } from '@tanstack/react-query';
7+
import { useMemo, useRef, useState } from 'react';
78
import { useTranslation } from 'react-i18next';
89
import { Alert, View } from 'react-native';
910
import Animated, { FadeIn, FadeOut } from 'react-native-reanimated';
@@ -45,6 +46,7 @@ import { isPersonal, useReviewConfig } from '@/lib/hooks/use-code-reviewer';
4546
import { useRelativeTimeNow } from '@/lib/hooks/use-now-ticker';
4647
import { useCancelReview, useRetriggerReview, useReviewDetail } from '@/lib/hooks/use-code-reviews';
4748
import { trpcErrorVariant } from '@/lib/trpc-error';
49+
import { useTRPC } from '@/lib/trpc';
4850
import { cn, parseTimestamp, timeAgo } from '@/lib/utils';
4951

5052
const FINDINGS_PAGE_SIZE = 20;
@@ -98,6 +100,25 @@ export function ReviewDetailScreen({
98100
!(isPersonal(scope) && reviewPlatform === 'bitbucket');
99101
const reviewConfig = useReviewConfig(scope, reviewPlatform, { enabled: configQueryEnabled });
100102

103+
// The in-app pull-request reader is gated on the user's own GitHub
104+
// authorization, and a Code Reviewer user can own a review without one. The
105+
// read is the stored status, not a GitHub call. It is enabled from the same
106+
// URL-derived provider the button decides on, never the review row's column,
107+
// so a row whose column disagrees with its URL still decides on one source.
108+
const openPrDestination = useMemo(
109+
() => resolveCodeReviewerOpenPrDestination(loadedReview?.pr_url ?? ''),
110+
[loadedReview?.pr_url]
111+
);
112+
const needsGitHubAuthorization =
113+
isPersonal(scope) &&
114+
openPrDestination.kind === 'in-app' &&
115+
openPrDestination.platform === 'github';
116+
const trpc = useTRPC();
117+
const githubAuthorization = useQuery({
118+
...trpc.githubApps.getUserAuthorization.queryOptions(),
119+
enabled: needsGitHubAuthorization,
120+
});
121+
101122
function confirmCancel(onConfirm: () => void) {
102123
confirm({
103124
title: i18n.t('codeReviewer.reviewDetail.cancelTitle'),
@@ -235,19 +256,6 @@ export function ReviewDetailScreen({
235256
statusLabel={meta.label}
236257
/>
237258
<TabScreenScrollView className="flex-1" contentContainerClassName="px-6 gap-4 pt-4">
238-
{/* The transcript entry is the first row of the content, not a pinned band
239-
above the list: a fixed trigger between the header and the scroll view
240-
leaves a strip where a vertical drag never reaches the content. */}
241-
<Button
242-
variant="secondary"
243-
accessibilityLabel={t('agentChat.session.transcriptAccessibility')}
244-
onPress={() => {
245-
setTranscriptReviewId(reviewId);
246-
}}
247-
>
248-
<Text>{t('agentChat.session.transcriptAccessibility')}</Text>
249-
</Button>
250-
251259
{/* A background poll failure must not blank out an already-loaded review. */}
252260
{isError && (
253261
<Text variant="muted" className="text-center text-xs">
@@ -259,7 +267,6 @@ export function ReviewDetailScreen({
259267
<Text className="text-base font-medium">{review.pr_title}</Text>
260268
<Text variant="muted" className="text-xs">
261269
{t('codeReviewer.reviewDetail.byline', {
262-
repo: review.repo_full_name,
263270
number: review.pr_number,
264271
author: review.pr_author,
265272
})}
@@ -291,9 +298,13 @@ export function ReviewDetailScreen({
291298
<View className="gap-2">
292299
<Text className="text-sm font-medium">{t('common.findings')}</Text>
293300
{visibleFindings.length === 0 ? (
294-
<Text variant="muted" className="text-xs">
295-
{t(findingsEmptyCopyKey(review.status))}
296-
</Text>
301+
// The empty result sits in the same card as every other section's
302+
// body, so an ended review reads as a filled section, not a caption.
303+
<View className="rounded-lg bg-secondary p-4">
304+
<Text variant="muted" className="text-xs">
305+
{t(findingsEmptyCopyKey(review.status))}
306+
</Text>
307+
</View>
297308
) : (
298309
<View className="gap-2">
299310
{visibleFindings.map((finding, index) => (
@@ -316,11 +327,11 @@ export function ReviewDetailScreen({
316327
{/* Council: decision plus specialist names/votes as text. */}
317328
{review.council_result ? <CouncilSection councilResult={review.council_result} /> : null}
318329

319-
{/* Gate: check-run presence, review status, and threshold when set. */}
330+
{/* Gate: check-run presence and threshold when set. The review status is
331+
the conclusion above, so the card never repeats it. */}
320332
<GateSection
321333
checkRunId={review.check_run_id}
322334
checkRunRedacted={review.rawIdsRedacted}
323-
statusLabel={meta.label}
324335
gateThreshold={gateThreshold}
325336
/>
326337

@@ -369,17 +380,23 @@ export function ReviewDetailScreen({
369380
</View>
370381
</View>
371382

372-
{/* Actions. */}
383+
{/* Actions. The pull request is the screen's primary action, so it takes
384+
the filled primary style; every other action stays secondary. */}
373385
<View className="gap-3">
374386
<Button
375-
variant="secondary"
376387
onPress={() => {
377-
if (isPersonal(scope)) {
378-
const destination = resolveCodeReviewerOpenPrDestination(review.pr_url);
379-
if (destination.kind === 'in-app') {
380-
pushOnce(destination.href);
381-
return;
382-
}
388+
// A github.com pull request reads in the app only with the user's
389+
// own GitHub authorization; without it that route is the Connect
390+
// wall, so the pull request opens in the browser instead. A
391+
// pending or failed read keeps that fallback. Other providers
392+
// keep their in-app route, whose own gate owns their connection.
393+
if (
394+
isPersonal(scope) &&
395+
openPrDestination.kind === 'in-app' &&
396+
(!needsGitHubAuthorization || githubAuthorization.data?.connected === true)
397+
) {
398+
pushOnce(openPrDestination.href);
399+
return;
383400
}
384401
void openExternalUrl(review.pr_url, {
385402
label: t('common.pullRequest'),
@@ -391,6 +408,18 @@ export function ReviewDetailScreen({
391408
<Text>{t('common.openPullRequest')}</Text>
392409
</Button>
393410

411+
{/* The transcript is a secondary way to read the same review, so it
412+
sits beside the primary action and never above the title. */}
413+
<Button
414+
variant="secondary"
415+
accessibilityLabel={t('agentChat.session.transcriptAccessibility')}
416+
onPress={() => {
417+
setTranscriptReviewId(reviewId);
418+
}}
419+
>
420+
<Text>{t('agentChat.session.transcriptAccessibility')}</Text>
421+
</Button>
422+
394423
{canCancel ? (
395424
<Button
396425
variant="destructive"

‎apps/mobile/src/components/code-reviewer/review-detail-sections.tsx‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -90,12 +90,10 @@ export function CouncilSection({ councilResult }: Readonly<{ councilResult: Coun
9090
export function GateSection({
9191
checkRunId,
9292
checkRunRedacted,
93-
statusLabel,
9493
gateThreshold,
9594
}: Readonly<{
9695
checkRunId: number | null;
9796
checkRunRedacted?: boolean;
98-
statusLabel: string;
9997
gateThreshold?: string;
10098
}>) {
10199
const { t } = useTranslation();
@@ -110,7 +108,6 @@ export function GateSection({
110108
<Text className="text-sm font-medium">{t('codeReviewer.reviewDetail.gate')}</Text>
111109
<View className="gap-1 rounded-lg bg-secondary p-4">
112110
<MetaRow label={t('codeReviewer.reviewDetail.checkRun')} value={checkRunLabel} />
113-
<MetaRow label={t('common.status')} value={statusLabel} />
114111
{gateThreshold ? (
115112
<MetaRow
116113
label={t('codeReviewer.reviewDetail.threshold')}

‎apps/mobile/src/i18n/locales/af.json‎

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎apps/mobile/src/i18n/locales/am.json‎

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)