diff --git a/apps/mobile/e2e/AGENTS.md b/apps/mobile/e2e/AGENTS.md index 9f7db57102..b6ef20bda7 100644 --- a/apps/mobile/e2e/AGENTS.md +++ b/apps/mobile/e2e/AGENTS.md @@ -59,7 +59,19 @@ pnpm dev:capture mobile - Never guess a tmux window from `tmux ls` or address `:` directly; a service pane may be joined into the dashboard with no same-named window. Use `pnpm dev:capture `. - Use raw tmux only for an interactive process, after reading the exact `session` from `pnpm dev:status --json` and resolving the pane with `tmux list-panes -a`. - Put any extra long-lived CLI, recorder, or log follower in a clearly named `kilo-e2e-*` tmux session so it is visible and easy to remove. -- Never commit E2E fixtures. Generate them in a temporary directory (`mktemp -d`) and delete it before finishing. +- Never commit E2E fixtures. Generate them in a temporary directory (`mktemp -d`) and delete it before finishing. Committed harness code under `e2e/` (this directory's `login.sh`, `preflight.sh`, `remote-cli.sh`, and `github-api-stub/`) is not a generated fixture; the rule targets per-run generated data only. + +## Hermetic PR-review GitHub stub + +PR-review E2E needs a local GitHub API instead of `api.github.com`. Three steps, all reversible: + +1. Set `GITHUB_API_BASE_URL` in the **worktree root** `.env.local` to the stub's base URL (for example `http://127.0.0.1:`). Next.js reads it via `apps/web/src/lib/github-pr-review/client.ts` (the web `dev` script symlinks root `.env.local` into `apps/web/` when that file is missing). Remove the variable after the run. +2. Seed a GitHub user token with the dev-only tRPC mutation `githubApps.devSeedUserGithubToken`. Any non-empty token string works. Use the stable fake `githubUserId` `999001` so the upsert matches across worktrees; a `false` upsert result means a sibling already seeded it, not failure. +3. Start the stub in a `kilo-e2e-*` tmux session, for example: + `tmux new-session -d -s "kilo-e2e-github-stub-$(basename "$PWD")" -c "$PWD/apps/mobile/e2e/github-api-stub" "node server.mjs "`. + Stop that session when finished. Request logs go to `GITHUB_STUB_LOG` or `./github-api-stub-requests.log` under the process cwd — keep them out of the tree (temp dir) and delete them after the run. + +Pinned surface only: REST pull/repo/check-runs/statuses plus GraphQL ops `PrReviewDecision`, `PrReviewThreads`, `PrReviewThreadComments`, `PrReviewConversationComments`. Fixture identities: `kilo-stub/discussion-mixed#1`, `kilo-stub/discussion-conversation-only#2`, `kilo-stub/discussion-empty#3`. ## iOS Simulator @@ -95,11 +107,11 @@ xcrun simctl openurl \ Backend and Metro must be running. These idempotent wrappers verify simulator ownership, required services, the generated API port, and Metro project provenance, then reconnect the dev client to this worktree's exact Metro URL before Maestro runs. Never bypass their preflight or call the login YAML flows directly: ```bash -apps/mobile/e2e/login.sh [email] # default: e2e-mobile+@example.com +apps/mobile/e2e/login.sh [email] # default: e2e-mobile-@example.com apps/mobile/e2e/logout.sh ``` -The default email is unique per worktree, so concurrent worktrees sign into distinct backend users. Pass an explicit email only when a test needs a specific account. +The default email is `e2e-mobile-@example.com`, derived deterministically from the worktree directory name. Hyphens are preserved by `normalizeEmail`, so each worktree signs into a distinct backend user. Pass an explicit email only when a test needs a specific account. Login requests an email OTP, waits up to 30 seconds for the worktree-local outbox, verifies the code, accepts first-account consent, and asserts Home. It retries the known dev-client launch boundary once. `flows/settle-app.yaml` handles late tracking and Expo developer-menu prompts without restarting the app; `flows/open-app.yaml` is the standalone cold-launch flow. @@ -163,7 +175,7 @@ The orchestrator starts a local CLI as a remote session for this worktree: apps/mobile/e2e/remote-cli.sh start [email] ``` -The helper resolves this worktree's stack ports, mints a token for the given user (default: the per-worktree login account, `e2e-mobile+@example.com`), installs the CLI into a disposable per-worktree directory, and launches it in a `kilo-e2e-cli-` tmux session already pointed at the local API, session-ingest, and event-service. Pass the account the app is signed in as when it differs from the default. Manage it with `remote-cli.sh status` and `remote-cli.sh stop`. +The helper resolves this worktree's stack ports, mints a token for the given user (default: the per-worktree login account, `e2e-mobile-@example.com`), installs the CLI into a disposable per-worktree directory, and launches it in a `kilo-e2e-cli-` tmux session already pointed at the local API, session-ingest, and event-service. Pass the account the app is signed in as when it differs from the default. Manage it with `remote-cli.sh status` and `remote-cli.sh stop`. Run any one-off CLI command against the same prepared stack with `exec` instead of the interactive TUI: diff --git a/apps/mobile/e2e/github-api-stub/server.mjs b/apps/mobile/e2e/github-api-stub/server.mjs new file mode 100644 index 0000000000..d20907f4ca --- /dev/null +++ b/apps/mobile/e2e/github-api-stub/server.mjs @@ -0,0 +1,445 @@ +#!/usr/bin/env node +/** + * Hermetic local GitHub API stub for mobile PR-review E2E. + * Node built-ins only. Logs every request; GraphQL logs operation name + variables. + * + * Identities: + * kilo-stub/discussion-mixed#1 — interleaved review + conversation fixture + * kilo-stub/discussion-conversation-only#2 — conversation comments only (0 review threads) + * kilo-stub/discussion-empty#3 — empty discussion + */ +import http from 'node:http'; +import fs from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const PORT = Number(process.env.GITHUB_STUB_PORT || process.argv[2] || 0); +const LOG_PATH = + process.env.GITHUB_STUB_LOG || path.join(process.cwd(), 'github-api-stub-requests.log'); + +const T0 = '2026-03-01T12:00:00.000Z'; +// Interleaved timeline (T+minutes from T0): +// T+1 threadA c1, T+2 conv1, T+3 conv2, T+4 threadB c1, T+5 threadA c2, T+6 conv3 +const ts = minutes => { + const d = new Date(T0); + d.setUTCMinutes(d.getUTCMinutes() + minutes); + return d.toISOString(); +}; + +const AVATAR = 'https://avatars.githubusercontent.com/u/1?v=4'; +const author = login => ({ login, avatarUrl: AVATAR }); +const restUser = login => ({ + login, + id: 1, + node_id: 'U_stub', + avatar_url: AVATAR, + html_url: `https://github.com/${login}`, + type: 'User', + site_admin: false, +}); + +const reactionGroups = () => + ['THUMBS_UP', 'THUMBS_DOWN', 'LAUGH', 'HOORAY', 'CONFUSED', 'HEART', 'ROCKET', 'EYES'].map( + content => ({ + content, + viewerHasReacted: false, + reactors: { totalCount: 0 }, + }) + ); + +/** GraphQL IssueComment node for pullRequest.comments (PrReviewConversationComments). */ +const conversationComment = (databaseId, login, body, minutes) => ({ + id: `IC_stub_${databaseId}`, + databaseId, + author: author(login), + body, + createdAt: ts(minutes), + reactionGroups: reactionGroups(), +}); + +/** @type {Record} */ +const FIXTURES = { + 'kilo-stub/discussion-mixed/1': { + title: 'Mixed discussion fixture', + body: 'PR body for mixed fixture.', + threads: [ + { + id: 'PRRT_thread_a', + isResolved: false, + isOutdated: false, + subjectType: 'LINE', + path: 'src/alpha.ts', + line: 10, + startLine: 10, + originalLine: 10, + originalStartLine: 10, + diffSide: 'RIGHT', + comments: { + pageInfo: { hasNextPage: false, endCursor: null }, + nodes: [ + { + databaseId: 1001, + id: 'PRRC_a1', + body: 'Thread A comment at T+1 (inline review)', + createdAt: ts(1), + author: author('alice'), + reactionGroups: reactionGroups(), + }, + { + databaseId: 1005, + id: 'PRRC_a2', + body: 'Thread A reply at T+5 (inline review)', + createdAt: ts(5), + author: author('bob'), + reactionGroups: reactionGroups(), + }, + ], + }, + }, + { + id: 'PRRT_thread_b', + isResolved: false, + isOutdated: false, + subjectType: 'LINE', + path: 'src/beta.ts', + line: 20, + startLine: 20, + originalLine: 20, + originalStartLine: 20, + diffSide: 'RIGHT', + comments: { + pageInfo: { hasNextPage: false, endCursor: null }, + nodes: [ + { + databaseId: 1004, + id: 'PRRC_b1', + body: 'Thread B comment at T+4 (inline review)', + createdAt: ts(4), + author: author('carol'), + reactionGroups: reactionGroups(), + }, + ], + }, + }, + ], + // GraphQL pullRequest.comments nodes. REST issues/{n}/comments is intentionally + // not served — S2 reads conversation comments over GraphQL only. + conversationComments: [ + conversationComment(2002, 'dave', 'Conversation comment at T+2', 2), + conversationComment(2003, 'erin', 'Conversation comment at T+3', 3), + conversationComment(2006, 'frank', 'Conversation comment at T+6', 6), + ], + }, + 'kilo-stub/discussion-conversation-only/2': { + title: 'Conversation-only fixture', + body: 'PR body for conversation-only fixture.', + threads: [], + conversationComments: [ + conversationComment(3001, 'dave', 'Only conversation comment one', 2), + conversationComment(3002, 'erin', 'Only conversation comment two', 6), + ], + }, + 'kilo-stub/discussion-empty/3': { + title: 'Empty discussion fixture', + body: 'PR body for empty fixture.', + threads: [], + conversationComments: [], + }, +}; + +function logLine(obj) { + const line = JSON.stringify({ t: new Date().toISOString(), ...obj }); + fs.appendFileSync(LOG_PATH, line + '\n'); + console.log(line); +} + +function parseOpName(query) { + if (typeof query !== 'string') return null; + const m = query.match(/\b(?:query|mutation)\s+([A-Za-z_][A-Za-z0-9_]*)/); + return m ? m[1] : null; +} + +function fixtureKey(owner, repo, number) { + return `${owner}/${repo}/${number}`; +} + +function getFixture(owner, repo, number) { + return FIXTURES[fixtureKey(owner, repo, number)] ?? null; +} + +function restPull(owner, repo, number, fx) { + const full = `${owner}/${repo}`; + const sha = 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'; + return { + id: Number(number) * 1000, + node_id: `PR_kwstub_${owner}_${repo}_${number}`, + number: Number(number), + title: fx.title, + body: fx.body, + state: 'open', + locked: false, + draft: false, + merged: false, + mergeable: true, + mergeable_state: 'clean', + auto_merge: null, + commits: 1, + changed_files: 2, + additions: 10, + deletions: 2, + user: restUser('alice'), + head: { + ref: 'feature/stub', + sha, + repo: { + id: 1, + node_id: 'R_head', + name: repo, + full_name: full, + private: false, + owner: restUser(owner), + }, + }, + base: { + ref: 'main', + sha: 'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb', + repo: { + id: 1, + node_id: 'R_base', + name: repo, + full_name: full, + private: false, + owner: restUser(owner), + }, + }, + html_url: `https://github.com/${full}/pull/${number}`, + created_at: T0, + updated_at: T0, + }; +} + +function restRepo(owner, repo) { + return { + id: 1, + node_id: 'R_stub', + name: repo, + full_name: `${owner}/${repo}`, + private: false, + owner: restUser(owner), + allow_merge_commit: true, + allow_squash_merge: true, + allow_rebase_merge: true, + allow_auto_merge: false, + delete_branch_on_merge: false, + allow_update_branch: true, + permissions: { admin: true, push: true, pull: true }, + default_branch: 'main', + }; +} + +function json(res, status, body) { + const payload = JSON.stringify(body); + res.writeHead(status, { + 'Content-Type': 'application/json', + 'Content-Length': Buffer.byteLength(payload), + // Intentionally omit Link headers so octokit.paginate does not follow out. + }); + res.end(payload); +} + +function handleGraphql(body, res) { + let parsed; + try { + parsed = JSON.parse(body || '{}'); + } catch { + logLine({ method: 'POST', path: '/graphql', error: 'invalid_json' }); + return json(res, 400, { message: 'invalid json' }); + } + const op = parseOpName(parsed.query); + const variables = parsed.variables ?? {}; + logLine({ + method: 'POST', + path: '/graphql', + operationName: op, + variables, + queryPreview: typeof parsed.query === 'string' ? parsed.query.slice(0, 400) : null, + // Full query text for D1 evidence when it is PrReviewThreads + query: op === 'PrReviewThreads' || op === 'PrReviewDecision' ? parsed.query : undefined, + }); + + if (op === 'PrReviewDecision') { + return json(res, 200, { + data: { + repository: { + pullRequest: { reviewDecision: null }, + }, + viewer: { login: 'kilo-stub-user' }, + }, + }); + } + + // Overview enrichment query name may differ — serve reviewDecision + viewer for any + // query that looks like the overview fragment. + if (op && /Decision|Overview|Fragment|PullRequest/i.test(op) && op !== 'PrReviewThreads') { + // Prefer matching known names; still return a safe shape. + if (op !== 'PrReviewThreadComments') { + return json(res, 200, { + data: { + repository: { + pullRequest: { reviewDecision: null }, + }, + viewer: { login: 'kilo-stub-user' }, + }, + }); + } + } + + if (op === 'PrReviewThreads') { + const owner = variables.owner; + const name = variables.name; + const number = variables.number; + const fx = getFixture(owner, name, number); + if (!fx) { + return json(res, 200, { + data: { repository: { pullRequest: null } }, + }); + } + return json(res, 200, { + data: { + repository: { + pullRequest: { + reviewThreads: { + pageInfo: { hasNextPage: false, endCursor: null }, + nodes: fx.threads, + }, + }, + }, + }, + }); + } + + if (op === 'PrReviewThreadComments') { + return json(res, 200, { + data: { + node: { + comments: { + pageInfo: { hasNextPage: false, endCursor: null }, + nodes: [], + }, + }, + }, + }); + } + + if (op === 'PrReviewConversationComments') { + const owner = variables.owner; + const name = variables.name; + const number = variables.number; + const fx = getFixture(owner, name, number); + if (!fx) { + return json(res, 200, { + data: { repository: { pullRequest: null } }, + }); + } + // Fixtures have ≤3 comments; always one page. first/after slicing is not honored. + return json(res, 200, { + data: { + repository: { + pullRequest: { + comments: { + pageInfo: { hasNextPage: false, endCursor: null }, + nodes: fx.conversationComments, + }, + }, + }, + }, + }); + } + + // Unknown GraphQL — non-401 so retry path does not rotate tokens. + return json(res, 200, { + data: null, + errors: [{ message: `stub: unhandled GraphQL operation ${op ?? 'unknown'}` }], + }); +} + +function readBody(req) { + return new Promise((resolve, reject) => { + const chunks = []; + req.on('data', c => chunks.push(c)); + req.on('end', () => resolve(Buffer.concat(chunks).toString('utf8'))); + req.on('error', reject); + }); +} + +const server = http.createServer(async (req, res) => { + const url = new URL(req.url || '/', `http://127.0.0.1`); + const method = req.method || 'GET'; + const pathname = url.pathname; + + try { + if (method === 'POST' && (pathname === '/graphql' || pathname === '/api/graphql')) { + const body = await readBody(req); + return handleGraphql(body, res); + } + + // GET /repos/{owner}/{repo}/pulls/{number} + let m = pathname.match(/^\/repos\/([^/]+)\/([^/]+)\/pulls\/(\d+)$/); + if (method === 'GET' && m) { + const [, owner, repo, number] = m; + logLine({ method, path: pathname, owner, repo, number }); + const fx = getFixture(owner, repo, number); + if (!fx) return json(res, 404, { message: 'Not Found' }); + return json(res, 200, restPull(owner, repo, number, fx)); + } + + // GET /repos/{owner}/{repo}/commits/{ref}/check-runs + m = pathname.match(/^\/repos\/([^/]+)\/([^/]+)\/commits\/([^/]+)\/check-runs$/); + if (method === 'GET' && m) { + logLine({ method, path: pathname }); + return json(res, 200, { total_count: 0, check_runs: [] }); + } + + // GET /repos/{owner}/{repo}/commits/{ref}/statuses + m = pathname.match(/^\/repos\/([^/]+)\/([^/]+)\/commits\/([^/]+)\/statuses$/); + if (method === 'GET' && m) { + logLine({ method, path: pathname }); + return json(res, 200, []); + } + + // GET /repos/{owner}/{repo} + m = pathname.match(/^\/repos\/([^/]+)\/([^/]+)$/); + if (method === 'GET' && m) { + const [, owner, repo] = m; + logLine({ method, path: pathname, owner, repo }); + return json(res, 200, restRepo(owner, repo)); + } + + // GET /user (sometimes used) + if (method === 'GET' && pathname === '/user') { + logLine({ method, path: pathname }); + return json(res, 200, restUser('kilo-stub-user')); + } + + logLine({ method, path: pathname, unhandled: true }); + return json(res, 404, { message: `stub: unhandled ${method} ${pathname}` }); + } catch (err) { + logLine({ method, path: pathname, error: String(err) }); + // Never 401 + return json(res, 500, { message: 'stub internal error' }); + } +}); + +server.listen(PORT, '127.0.0.1', () => { + const addr = server.address(); + const port = typeof addr === 'object' && addr ? addr.port : PORT; + console.log( + JSON.stringify({ + event: 'listen', + port, + logPath: LOG_PATH, + fixtures: Object.keys(FIXTURES), + }) + ); +}); diff --git a/apps/mobile/e2e/login.sh b/apps/mobile/e2e/login.sh index 759caf66c2..b2ba3e2884 100755 --- a/apps/mobile/e2e/login.sh +++ b/apps/mobile/e2e/login.sh @@ -9,7 +9,7 @@ # e2e/login.sh [email] # # When no email is given, defaults to a per-worktree-unique address -# (e2e-mobile+@example.com) so concurrent worktrees never +# (e2e-mobile-@example.com) so concurrent worktrees never # share a backend user. It is stable within a worktree, so repeat logins reuse # the same seeded account. # @@ -25,8 +25,8 @@ SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" REPO_ROOT="$(git -C "$SCRIPT_DIR" rev-parse --show-toplevel)" OUTBOX="${OUTBOX:-$REPO_ROOT/dev/logs/emails}" -WORKTREE_SLUG="$(basename "$REPO_ROOT" | tr -cs 'a-zA-Z0-9' '-' | sed 's/^-*//;s/-*$//')" -EMAIL="${2:-e2e-mobile+${WORKTREE_SLUG}@example.com}" +WORKTREE_SLUG="$(basename "$REPO_ROOT" | tr -cs 'a-zA-Z0-9' '-' | sed 's/^-*//;s/-*$//' | tr 'A-Z' 'a-z')" +EMAIL="${2:-e2e-mobile-${WORKTREE_SLUG}@example.com}" "$SCRIPT_DIR/preflight.sh" "$DEVICE" diff --git a/apps/mobile/e2e/remote-cli.sh b/apps/mobile/e2e/remote-cli.sh index c2fd7879a4..78d31423cf 100755 --- a/apps/mobile/e2e/remote-cli.sh +++ b/apps/mobile/e2e/remote-cli.sh @@ -21,7 +21,7 @@ # apps/mobile/e2e/remote-cli.sh exec run "say hello" # # When no email is given, defaults to the per-worktree-unique login account -# (e2e-mobile+@example.com), matching e2e/login.sh. The user must +# (e2e-mobile-@example.com), matching e2e/login.sh. The user must # already exist (sign in on the device first, or seed one). Pass an explicit # email to target a specific account (e.g. the one the app is signed in as). # `exec` reuses an already-prepared env and only prepares (mints a token) when @@ -37,7 +37,7 @@ set -euo pipefail SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" REPO_ROOT="$(git -C "$SCRIPT_DIR" rev-parse --show-toplevel)" WORKTREE_SLUG="$(basename "$REPO_ROOT" | tr -cs 'a-zA-Z0-9' '-' | sed 's/^-*//;s/-*$//')" -DEFAULT_EMAIL="e2e-mobile+${WORKTREE_SLUG}@example.com" +DEFAULT_EMAIL="e2e-mobile-$(printf '%s' "$WORKTREE_SLUG" | tr 'A-Z' 'a-z')@example.com" SESSION="kilo-e2e-cli-${WORKTREE_SLUG}" CLI_HOME="$REPO_ROOT/dev/.dev-logs/remote-cli/${WORKTREE_SLUG}" ENV_FILE="$CLI_HOME/.cli-env" diff --git a/apps/mobile/src/components/code-reviewer/review-detail-screen.tsx b/apps/mobile/src/components/code-reviewer/review-detail-screen.tsx index 31d802fe86..0ed8b1a094 100644 --- a/apps/mobile/src/components/code-reviewer/review-detail-screen.tsx +++ b/apps/mobile/src/components/code-reviewer/review-detail-screen.tsx @@ -1,4 +1,5 @@ import * as Haptics from 'expo-haptics'; +import { useRouter } from 'expo-router'; import { Alert, View } from 'react-native'; import Animated, { FadeIn, FadeOut } from 'react-native-reanimated'; @@ -14,9 +15,12 @@ import { Button } from '@/components/ui/button'; import { Skeleton } from '@/components/ui/skeleton'; import { Text } from '@/components/ui/text'; import { TabScreenScrollView } from '@/components/tab-screen'; +import { FEATURE_FLAG_PR_REVIEW, useFeatureFlag } from '@/lib/analytics/posthog'; +import { resolveCodeReviewerOpenPrDestination } from '@/lib/code-reviewer-open-pr-destination'; import { reviewerPlatformLabel } from '@/lib/code-reviewer-config'; import { openExternalUrl } from '@/lib/external-link'; import { useCancelReview, useRetriggerReview, useReviewDetail } from '@/lib/hooks/use-code-reviews'; +import { getPrReviewPath } from '@/lib/profile-agent-navigation'; import { cn, parseTimestamp, timeAgo } from '@/lib/utils'; function MetaRow({ @@ -52,6 +56,8 @@ export function ReviewDetailScreen({ scope, reviewId, }: Readonly<{ scope: string; reviewId: string }>) { + const router = useRouter(); + const prReviewEnabled = useFeatureFlag(FEATURE_FLAG_PR_REVIEW, true); const { data, isLoading, isError, isFetching, error, refetch } = useReviewDetail(reviewId); const cancelReview = useCancelReview(scope); const retriggerReview = useRetriggerReview(scope); @@ -158,6 +164,16 @@ export function ReviewDetailScreen({ - {/* - Reserved-height slot: always mounted at min-h-5 (one text-sm line) - so Open/Recent never shift when helper content swaps. - */} - {helperContent} - diff --git a/apps/mobile/src/lib/code-reviewer-open-pr-destination.test.ts b/apps/mobile/src/lib/code-reviewer-open-pr-destination.test.ts new file mode 100644 index 0000000000..4984f79dd8 --- /dev/null +++ b/apps/mobile/src/lib/code-reviewer-open-pr-destination.test.ts @@ -0,0 +1,52 @@ +import { describe, expect, it } from 'vitest'; + +import { resolveCodeReviewerOpenPrDestination } from './code-reviewer-open-pr-destination'; + +describe('resolveCodeReviewerOpenPrDestination', () => { + it('routes a valid github.com PR URL in-app when the flag is on', () => { + expect( + resolveCodeReviewerOpenPrDestination('https://github.com/octocat/hello-world/pull/42', true) + ).toEqual({ + kind: 'in-app', + owner: 'octocat', + repo: 'hello-world', + number: 42, + }); + }); + + it('opens the browser for a GitLab PR URL', () => { + expect( + resolveCodeReviewerOpenPrDestination('https://gitlab.com/octocat/hello-world/pull/42', true) + ).toEqual({ kind: 'browser' }); + }); + + it('opens the browser for a Bitbucket PR URL', () => { + expect( + resolveCodeReviewerOpenPrDestination( + 'https://bitbucket.org/octocat/hello-world/pull-requests/42', + true + ) + ).toEqual({ kind: 'browser' }); + }); + + it('opens the browser for a GitHub Enterprise host', () => { + expect( + resolveCodeReviewerOpenPrDestination( + 'https://github.example.com/octocat/hello-world/pull/42', + true + ) + ).toEqual({ kind: 'browser' }); + }); + + it('opens the browser for a malformed URL', () => { + expect(resolveCodeReviewerOpenPrDestination('not a url at all', true)).toEqual({ + kind: 'browser', + }); + }); + + it('opens the browser when the flag is off even for a valid github.com PR URL', () => { + expect( + resolveCodeReviewerOpenPrDestination('https://github.com/octocat/hello-world/pull/42', false) + ).toEqual({ kind: 'browser' }); + }); +}); diff --git a/apps/mobile/src/lib/code-reviewer-open-pr-destination.ts b/apps/mobile/src/lib/code-reviewer-open-pr-destination.ts new file mode 100644 index 0000000000..891175d125 --- /dev/null +++ b/apps/mobile/src/lib/code-reviewer-open-pr-destination.ts @@ -0,0 +1,26 @@ +import { parseGitHubPrUrl } from '@/lib/github-pr-url'; + +type CodeReviewerOpenPrDestination = + | { kind: 'in-app'; owner: string; repo: string; number: number } + | { kind: 'browser' }; + +/** + * Decide whether "Open pull request" should navigate in-app or open the browser. + * + * In-app only when the PR-review feature flag is on and `prUrl` is a parseable + * github.com PR URL. Everything else (flag off, Enterprise/GitLab/Bitbucket, + * malformed) keeps today's browser path. + */ +export function resolveCodeReviewerOpenPrDestination( + prUrl: string, + prReviewEnabled: boolean +): CodeReviewerOpenPrDestination { + if (!prReviewEnabled) { + return { kind: 'browser' }; + } + const parsed = parseGitHubPrUrl(prUrl); + if (!parsed) { + return { kind: 'browser' }; + } + return { kind: 'in-app', owner: parsed.owner, repo: parsed.repo, number: parsed.number }; +} diff --git a/apps/mobile/src/lib/organization-invoice-download.test.ts b/apps/mobile/src/lib/organization-invoice-download.test.ts new file mode 100644 index 0000000000..00812f03d7 --- /dev/null +++ b/apps/mobile/src/lib/organization-invoice-download.test.ts @@ -0,0 +1,110 @@ +import { describe, expect, it, vi } from 'vitest'; + +import { + getInvoiceDownloadErrorMessage, + getInvoicePdfFilename, + INVOICE_DOWNLOAD_FAILED_MESSAGE, + INVOICE_SHARING_UNAVAILABLE_MESSAGE, + selectInvoiceDownloadErrorMessage, + selectInvoiceRowState, +} from '@/lib/organization-invoice-download'; +import { ShareRemoteFileError } from '@/lib/share-remote-file'; + +vi.mock('expo-file-system', () => ({ + Directory: vi.fn(), + File: Object.assign(vi.fn(), { downloadFileAsync: vi.fn() }), + Paths: { cache: 'file:///cache' }, +})); + +vi.mock('expo-sharing', () => ({ + isAvailableAsync: vi.fn(), + shareAsync: vi.fn(), +})); + +vi.mock('react-native', () => ({ + Platform: { OS: 'ios' }, +})); + +describe('selectInvoiceRowState', () => { + it('selects no-affordance when invoice_pdf is null', () => { + expect(selectInvoiceRowState({ invoicePdf: null, sharing: false })).toBe('no-affordance'); + expect(selectInvoiceRowState({ invoicePdf: null, sharing: true })).toBe('no-affordance'); + }); + + it('selects busy while a download/share is in flight', () => { + expect( + selectInvoiceRowState({ + invoicePdf: 'https://files.stripe.com/invoices/inv_1.pdf', + sharing: true, + }) + ).toBe('busy'); + }); + + it('selects idle when a PDF is available and nothing is in flight', () => { + expect( + selectInvoiceRowState({ + invoicePdf: 'https://files.stripe.com/invoices/inv_1.pdf', + sharing: false, + }) + ).toBe('idle'); + }); +}); + +describe('selectInvoiceDownloadErrorMessage', () => { + it('maps download-failed to a retryable download message with no CTA', () => { + expect(selectInvoiceDownloadErrorMessage('download-failed')).toBe( + INVOICE_DOWNLOAD_FAILED_MESSAGE + ); + // Retry is by tapping the row again; the toast itself has no action CTA. + expect(INVOICE_DOWNLOAD_FAILED_MESSAGE.toLowerCase()).toContain('try again'); + }); + + it('maps sharing-unavailable to a distinct terminal message with no retry CTA', () => { + expect(selectInvoiceDownloadErrorMessage('sharing-unavailable')).toBe( + INVOICE_SHARING_UNAVAILABLE_MESSAGE + ); + expect(INVOICE_SHARING_UNAVAILABLE_MESSAGE.toLowerCase()).toContain('not available'); + expect(INVOICE_SHARING_UNAVAILABLE_MESSAGE.toLowerCase()).not.toContain('try again'); + }); +}); + +describe('getInvoiceDownloadErrorMessage', () => { + it('reads the discriminable reason from ShareRemoteFileError', () => { + expect(getInvoiceDownloadErrorMessage(new ShareRemoteFileError('sharing-unavailable'))).toBe( + INVOICE_SHARING_UNAVAILABLE_MESSAGE + ); + expect(getInvoiceDownloadErrorMessage(new ShareRemoteFileError('download-failed'))).toBe( + INVOICE_DOWNLOAD_FAILED_MESSAGE + ); + }); + + it('falls back to the download message for unknown errors', () => { + expect(getInvoiceDownloadErrorMessage(new Error('boom'))).toBe(INVOICE_DOWNLOAD_FAILED_MESSAGE); + }); +}); + +describe('getInvoicePdfFilename', () => { + it('prefers number, then description, then id, and appends .pdf', () => { + expect( + getInvoicePdfFilename({ + id: 'in_1', + number: 'INV-100', + description: 'Seat invoice', + }) + ).toBe('INV-100.pdf'); + expect( + getInvoicePdfFilename({ + id: 'in_1', + number: null, + description: 'Seat invoice', + }) + ).toBe('Seat invoice.pdf'); + expect(getInvoicePdfFilename({ id: 'in_1', number: null, description: null })).toBe('in_1.pdf'); + }); + + it('does not double-append .pdf', () => { + expect(getInvoicePdfFilename({ id: 'in_1', number: 'report.pdf', description: null })).toBe( + 'report.pdf' + ); + }); +}); diff --git a/apps/mobile/src/lib/organization-invoice-download.ts b/apps/mobile/src/lib/organization-invoice-download.ts new file mode 100644 index 0000000000..4a8059d8fa --- /dev/null +++ b/apps/mobile/src/lib/organization-invoice-download.ts @@ -0,0 +1,79 @@ +import { + getShareRemoteFileReason, + shareRemoteFile, + type ShareRemoteFileReason, +} from '@/lib/share-remote-file'; +import { firstNonEmpty } from '@/lib/utils'; + +const INVOICE_CACHE_DIRECTORY = 'org-invoices'; + +type InvoiceRowState = 'idle' | 'busy' | 'no-affordance'; + +export const INVOICE_DOWNLOAD_FAILED_MESSAGE = + "Couldn't download invoice. Check your connection and try again."; + +export const INVOICE_SHARING_UNAVAILABLE_MESSAGE = 'Sharing is not available on this device.'; + +type InvoiceRowStateInput = { + readonly invoicePdf: string | null; + readonly sharing: boolean; +}; + +/** + * Visual state for an organization invoice row. + * + * Exactly three states: no PDF → no affordance; sharing in flight → busy; + * otherwise idle. Failures are transient (toast then idle) and are not modeled + * as a fourth row state. + */ +export function selectInvoiceRowState(input: InvoiceRowStateInput): InvoiceRowState { + if (input.invoicePdf === null) { + return 'no-affordance'; + } + if (input.sharing) { + return 'busy'; + } + return 'idle'; +} + +/** + * Toast copy for a failed invoice download/share. + * + * `download-failed` is retryable by tapping the row again (no toast CTA). + * `sharing-unavailable` is terminal: the message says sharing is not available + * and there is no retry CTA. + */ +export function selectInvoiceDownloadErrorMessage(reason: ShareRemoteFileReason): string { + if (reason === 'sharing-unavailable') { + return INVOICE_SHARING_UNAVAILABLE_MESSAGE; + } + return INVOICE_DOWNLOAD_FAILED_MESSAGE; +} + +export function getInvoiceDownloadErrorMessage(error: unknown): string { + const reason = getShareRemoteFileReason(error) ?? 'download-failed'; + return selectInvoiceDownloadErrorMessage(reason); +} + +export function getInvoicePdfFilename(invoice: { + readonly id: string; + readonly number: string | null; + readonly description?: string | null; +}): string { + const stem = firstNonEmpty(invoice.number, invoice.description, invoice.id); + return stem.toLowerCase().endsWith('.pdf') ? stem : `${stem}.pdf`; +} + +export async function shareOrganizationInvoicePdf(invoice: { + readonly id: string; + readonly number: string | null; + readonly description?: string | null; + readonly invoice_pdf: string; +}): Promise { + await shareRemoteFile({ + url: invoice.invoice_pdf, + cacheDirectoryName: INVOICE_CACHE_DIRECTORY, + cacheKey: invoice.id, + filename: getInvoicePdfFilename(invoice), + }); +} diff --git a/apps/mobile/src/lib/pr-review/discussion/review-discussion-merge.test.ts b/apps/mobile/src/lib/pr-review/discussion/review-discussion-merge.test.ts new file mode 100644 index 0000000000..8cf8336fb0 --- /dev/null +++ b/apps/mobile/src/lib/pr-review/discussion/review-discussion-merge.test.ts @@ -0,0 +1,180 @@ +import { describe, expect, it } from 'vitest'; + +import { + compareDiscussionListItems, + type ConversationComment, + type DiscussionListItem, + isDiscussionEmpty, + mergeDiscussionListItems, + type ReviewComment, + type ReviewThread, +} from './review-discussion-types'; + +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', + comments: [makeComment()], + ...overrides, + }; +} + +function makeConversation(overrides: Partial = {}): ConversationComment { + return makeComment({ nodeId: 'IC1', commentId: 100, ...overrides }); +} + +function keysOf(items: readonly DiscussionListItem[]): string[] { + return items.map(item => + item.kind === 'thread' ? `thread:${item.thread.threadId}` : `comment:${item.comment.nodeId}` + ); +} + +function threadAt( + threadId: string, + firstAt: string, + extraComments: ReviewComment[] = [] +): ReviewThread { + return makeThread({ + threadId, + comments: [makeComment({ nodeId: `${threadId}-1`, createdAt: firstAt }), ...extraComments], + }); +} + +describe('mergeDiscussionListItems', () => { + // A2.1: thread A @ T+1 (reply T+5 nested); conv T+2/T+3; thread B @ T+4; conv T+6. + it('merges threads and conversation into one ascending list (A2.1)', () => { + const threadA = threadAt('A', at(1), [ + makeComment({ nodeId: 'A2', commentId: 2, createdAt: at(5) }), + ]); + const threadB = threadAt('B', at(4)); + const ic2 = makeConversation({ nodeId: 'IC2', createdAt: at(2) }); + const ic3 = makeConversation({ nodeId: 'IC3', createdAt: at(3) }); + const ic6 = makeConversation({ nodeId: 'IC6', createdAt: at(6) }); + + // Scrambled input so page-arrival order cannot leak through. + const merged = mergeDiscussionListItems([threadB, threadA], [ic6, ic2, ic3]); + expect(keysOf(merged)).toEqual([ + 'thread:A', + 'comment:IC2', + 'comment:IC3', + 'thread:B', + 'comment:IC6', + ]); + // Thread A once; T+5 reply stays nested (not an outer row). + const aRows = merged.filter(i => i.kind === 'thread' && i.thread.threadId === 'A'); + expect(aRows).toHaveLength(1); + const aOnly = aRows[0]; + expect(aOnly?.kind === 'thread' ? aOnly.thread.comments.map(c => c.nodeId) : []).toEqual([ + 'A-1', + 'A2', + ]); + }); + + it('renders conversation-only PRs (A2.6)', () => { + const merged = mergeDiscussionListItems( + [], + [ + makeConversation({ nodeId: 'IC1', createdAt: at(2) }), + makeConversation({ nodeId: 'IC0', createdAt: at(1) }), + ] + ); + expect(keysOf(merged)).toEqual(['comment:IC0', 'comment:IC1']); + }); + + it('keeps zero-comment threads after timestamped items (A2.2)', () => { + const empty = makeThread({ threadId: 'empty', comments: [] }); + const timed = threadAt('timed', at(1)); + const conv = makeConversation({ nodeId: 'IC', createdAt: at(2) }); + expect(keysOf(mergeDiscussionListItems([empty, timed], [conv]))).toEqual([ + 'thread:timed', + 'comment:IC', + 'thread:empty', + ]); + }); + + it('sorts unparseable first-comment timestamps last (A2.2)', () => { + const bad = makeThread({ + threadId: 'bad', + comments: [makeComment({ nodeId: 'badC', createdAt: 'not-a-date' })], + }); + const good = threadAt('good', at(1)); + expect(keysOf(mergeDiscussionListItems([bad, good], []))).toEqual([ + 'thread:good', + 'thread:bad', + ]); + }); + + it('on equal createdAt, threads before conversation comments (A2.2)', () => { + const stamp = at(10); + expect( + keysOf( + mergeDiscussionListItems( + [threadAt('T', stamp)], + [makeConversation({ nodeId: 'IC', createdAt: stamp })] + ) + ) + ).toEqual(['thread:T', 'comment:IC']); + }); + + it('on equal createdAt and kind, ties break on identity (A2.2)', () => { + const stamp = at(10); + expect( + keysOf( + mergeDiscussionListItems( + [threadAt('B', stamp), threadAt('A', stamp)], + [ + makeConversation({ nodeId: 'Z', createdAt: stamp }), + makeConversation({ nodeId: 'Y', createdAt: stamp }), + ] + ) + ) + ).toEqual(['thread:A', 'thread:B', 'comment:Y', 'comment:Z']); + }); + + it('full re-sort ignores input page order (R4)', () => { + // "Load more" can deliver an older thread after a newer one. + expect( + keysOf(mergeDiscussionListItems([threadAt('late', at(9)), threadAt('early', at(1))], [])) + ).toEqual(['thread:early', 'thread:late']); + }); +}); + +describe('compareDiscussionListItems', () => { + it('is antisymmetric on equal items', () => { + const item: DiscussionListItem = { kind: 'thread', thread: makeThread({ threadId: 'X' }) }; + expect(compareDiscussionListItems(item, item)).toBe(0); + }); +}); + +describe('isDiscussionEmpty', () => { + it('is true only when both kinds are absent (A2.5)', () => { + expect(isDiscussionEmpty([], [])).toBe(true); + expect(isDiscussionEmpty([makeThread()], [])).toBe(false); + expect(isDiscussionEmpty([], [makeConversation()])).toBe(false); + expect(isDiscussionEmpty([makeThread()], [makeConversation()])).toBe(false); + }); +}); diff --git a/apps/mobile/src/lib/pr-review/discussion/review-discussion-reducers.test.ts b/apps/mobile/src/lib/pr-review/discussion/review-discussion-reducers.test.ts index 820312bc72..f8c906dfb5 100644 --- a/apps/mobile/src/lib/pr-review/discussion/review-discussion-reducers.test.ts +++ b/apps/mobile/src/lib/pr-review/discussion/review-discussion-reducers.test.ts @@ -36,7 +36,7 @@ function makeThread(overrides: Partial = {}): ReviewThread { function makeData(threads: ReviewThread[]): ReviewThreadsInfiniteData { return { - pages: [{ threads, nextCursor: null }], + pages: [{ threads, conversation: [], nextCursor: null }], pageParams: [null], }; } @@ -68,8 +68,8 @@ describe('applyResolveToggle', () => { it('walks all pages, not just the first', () => { const data: ReviewThreadsInfiniteData = { pages: [ - { threads: [makeThread({ threadId: 'A' })], nextCursor: 'p2' }, - { threads: [makeThread({ threadId: 'B' })], nextCursor: null }, + { threads: [makeThread({ threadId: 'A' })], conversation: [], nextCursor: 'p2' }, + { threads: [makeThread({ threadId: 'B' })], conversation: [], nextCursor: null }, ], pageParams: [null, 'p2'], }; diff --git a/apps/mobile/src/lib/pr-review/discussion/review-discussion-types.test.ts b/apps/mobile/src/lib/pr-review/discussion/review-discussion-types.test.ts index fd4cbf390d..bbddd974bc 100644 --- a/apps/mobile/src/lib/pr-review/discussion/review-discussion-types.test.ts +++ b/apps/mobile/src/lib/pr-review/discussion/review-discussion-types.test.ts @@ -1,7 +1,6 @@ import { describe, expect, it } from 'vitest'; import { - groupThreadsByPath, type ReviewThread, selectCommentAuthorName, selectThreadAnchorLabel, @@ -174,22 +173,3 @@ describe('selectCommentAuthorName', () => { expect(selectCommentAuthorName(null)).toBe('deleted user'); }); }); - -describe('groupThreadsByPath', () => { - it('groups threads by path, preserving insertion order', () => { - const a = makeThread({ threadId: 'A', path: 'src/a.ts' }); - const b = makeThread({ threadId: 'B', path: 'src/b.ts' }); - const a2 = makeThread({ threadId: 'A2', path: 'src/a.ts' }); - const groups = groupThreadsByPath([a, b, a2]); - expect(groups.map(g => g.path)).toEqual(['src/a.ts', 'src/b.ts']); - expect(groups[0]?.threads.map(t => t.threadId)).toEqual(['A', 'A2']); - expect(groups[1]?.threads.map(t => t.threadId)).toEqual(['B']); - }); - - it('buckets null-path threads under "(no file)"', () => { - const orphan = makeThread({ threadId: 'X', path: null }); - const a = makeThread({ threadId: 'A', path: 'src/a.ts' }); - const groups = groupThreadsByPath([a, orphan]); - expect(groups.map(g => g.path)).toEqual(['src/a.ts', '(no file)']); - }); -}); diff --git a/apps/mobile/src/lib/pr-review/discussion/review-discussion-types.ts b/apps/mobile/src/lib/pr-review/discussion/review-discussion-types.ts index b11cabba48..49be01919f 100644 --- a/apps/mobile/src/lib/pr-review/discussion/review-discussion-types.ts +++ b/apps/mobile/src/lib/pr-review/discussion/review-discussion-types.ts @@ -22,6 +22,8 @@ import { type inferRouterOutputs, type RootRouter } from '@kilocode/trpc'; +import { parseTimestamp } from '@/lib/utils'; + // GitHub's 8 review-comment reaction content values. The trpc DTO // exposes this as a plain `string`; we narrow to the union here so // the reducer and the pill row get exhaustiveness checking. @@ -54,6 +56,8 @@ type RouterOutputs = inferRouterOutputs; export type ReviewThreadsPage = RouterOutputs['githubPrReview']['listReviewThreads']; export type ReviewThread = ReviewThreadsPage['threads'][number]; export type ReviewComment = ReviewThread['comments'][number]; +/** Conversation (issue) comments share the review-comment DTO shape. */ +export type ConversationComment = ReviewThreadsPage['conversation'][number]; // The shape of a single page in the cached `InfiniteData` // produced by `useInfiniteQuery(trpc.githubPrReview.listReviewThreads…)`. @@ -62,6 +66,16 @@ export type ReviewThreadsInfiniteData = { readonly pageParams: readonly unknown[]; }; +/** + * One row in the Discussion tab's merged chronological list. + * Threads stay intact as single items (nested comments do not get + * independent outer-list positions). Conversation comments are + * individual rows. + */ +export type DiscussionListItem = + | { readonly kind: 'thread'; readonly thread: ReviewThread } + | { readonly kind: 'comment'; readonly comment: ConversationComment }; + /** * Display label for a thread's anchor. The label reflects THREE * different nullable shapes: @@ -130,28 +144,89 @@ export function selectCommentAuthorName(author: ReviewComment['author']): string } /** - * Group threads by `path` for the sectioned list. Unanchored / null-path - * threads (rare but possible) are bucketed under "(no file)". The - * grouping is deterministic (Map insertion order = API order) so - * snapshots are stable across renders. + * Sort key timestamp for a discussion list item. + * - Thread: first comment's `createdAt` (later replies stay nested). + * - Conversation comment: its own `createdAt`. + * Returns `null` when missing or unparseable so those items sort after + * every item with a usable timestamp (A2.2 total-order rule). */ -type ReviewThreadGroup = { - readonly path: string; - readonly threads: readonly ReviewThread[]; -}; +function discussionItemTimestampMs(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; +} -export function groupThreadsByPath(threads: readonly ReviewThread[]): readonly ReviewThreadGroup[] { - const byPath = new Map(); - for (const thread of threads) { - const path = thread.path ?? '(no file)'; - const bucket = byPath.get(path); - if (bucket) { - bucket.push(thread); - } else { - byPath.set(path, [thread]); - } +function discussionItemIdentity(item: DiscussionListItem): string { + return item.kind === 'thread' ? item.thread.threadId : item.comment.nodeId; +} + +/** + * A2.2 total order over every DTO-legal input: + * 1. `createdAt` ascending (usable timestamps before missing/unparseable) + * 2. threads before conversation comments on a tie + * 3. stable string identity (`threadId` / `nodeId`) lexicographic + */ +export function compareDiscussionListItems(a: DiscussionListItem, b: DiscussionListItem): number { + const aMs = discussionItemTimestampMs(a); + const bMs = discussionItemTimestampMs(b); + const aHas = aMs !== null; + const bHas = bMs !== null; + if (aHas && bHas && aMs !== bMs) { + return aMs - bMs; + } + if (aHas !== bHas) { + return aHas ? -1 : 1; + } + const aKind = a.kind === 'thread' ? 0 : 1; + const bKind = b.kind === 'thread' ? 0 : 1; + if (aKind !== bKind) { + return aKind - bKind; } - return Array.from(byPath, ([path, group]) => ({ path, threads: group })); + const aId = discussionItemIdentity(a); + const bId = discussionItemIdentity(b); + if (aId < bId) { + return -1; + } + if (aId > bId) { + return 1; + } + return 0; +} + +/** + * Merge review threads and conversation comments into one ascending list. + * + * Full re-sort of the **entire loaded set** on every call (R4 / plan + * ordering rule): never rely on page arrival order (GitHub's + * `reviewThreads` connection has no ordering guarantee), and never sort + * only the newest page. A later "Load more" page can therefore insert + * rows mid-list; that is accepted. + * + * Conversation comments are complete after page one; callers should pass + * the first page's `conversation` (later pages are `[]` by contract). + */ +export function mergeDiscussionListItems( + threads: readonly ReviewThread[], + conversation: readonly ConversationComment[] +): readonly DiscussionListItem[] { + const items: DiscussionListItem[] = [ + ...threads.map(thread => ({ kind: 'thread' as const, thread })), + ...conversation.map(comment => ({ kind: 'comment' as const, comment })), + ]; + items.sort(compareDiscussionListItems); + return items; +} + +/** Empty only when neither threads nor conversation comments are present. */ +export function isDiscussionEmpty( + threads: readonly ReviewThread[], + conversation: readonly ConversationComment[] +): boolean { + return threads.length === 0 && conversation.length === 0; } /** diff --git a/apps/mobile/src/lib/pr-review/discussion/use-pr-review-discussion-threads.ts b/apps/mobile/src/lib/pr-review/discussion/use-pr-review-discussion-threads.ts index de76fa951a..15f635f32d 100644 --- a/apps/mobile/src/lib/pr-review/discussion/use-pr-review-discussion-threads.ts +++ b/apps/mobile/src/lib/pr-review/discussion/use-pr-review-discussion-threads.ts @@ -12,6 +12,10 @@ // AND the query has finished (no longer `isPending`). The // `firstPageErrorState` helper below encodes that distinction so // the tab UI doesn't have to. +// +// Conversation comments are returned on the first page only (backend +// contract: later pages carry `conversation: []`). We read page 0 +// only — simpler than flatten-then-dedupe and matches the guarantee. import { useInfiniteQuery } from '@tanstack/react-query'; @@ -44,13 +48,17 @@ export function usePrReviewDiscussionThreads(args: { const laterPageError = Boolean(query.error) && hasLoadedPages; // Flat list of all threads across all loaded pages, in page order. - // We return a fresh array on every render so the consumer can - // `.map` without memoizing; the rows themselves are stable. + // Ordering for display is applied by `mergeDiscussionListItems` in + // the tab (full re-sort of the entire loaded set). const threads = (query.data?.pages ?? []).flatMap(page => page.threads); + // First page only — backend guarantees later pages return []. + const conversation = query.data?.pages[0]?.conversation ?? []; + return { query, threads, + conversation, firstPageErrorState, laterPageError, }; diff --git a/apps/mobile/src/lib/pr-review/pr-link-helper-slot.test.ts b/apps/mobile/src/lib/pr-review/pr-link-helper-slot.test.ts index e1c9310c10..9939f39cf1 100644 --- a/apps/mobile/src/lib/pr-review/pr-link-helper-slot.test.ts +++ b/apps/mobile/src/lib/pr-review/pr-link-helper-slot.test.ts @@ -3,6 +3,7 @@ import { describe, expect, it } from 'vitest'; import { PR_LINK_HELPER_CLIPBOARD_EMPTY_COPY, PR_LINK_HELPER_INVALID_COPY, + selectPrLinkClearButtonVisible, selectPrLinkHelperSlotState, } from './pr-link-helper-slot'; @@ -30,3 +31,13 @@ describe('selectPrLinkHelperSlotState', () => { expect(PR_LINK_HELPER_CLIPBOARD_EMPTY_COPY).toBe('Clipboard is empty'); }); }); + +describe('selectPrLinkClearButtonVisible', () => { + it('is present when the field has content', () => { + expect(selectPrLinkClearButtonVisible({ hasInput: true })).toBe(true); + }); + + it('is absent when the field is empty', () => { + expect(selectPrLinkClearButtonVisible({ hasInput: false })).toBe(false); + }); +}); diff --git a/apps/mobile/src/lib/pr-review/pr-link-helper-slot.ts b/apps/mobile/src/lib/pr-review/pr-link-helper-slot.ts index 3ea42b63b0..8400a6d056 100644 --- a/apps/mobile/src/lib/pr-review/pr-link-helper-slot.ts +++ b/apps/mobile/src/lib/pr-review/pr-link-helper-slot.ts @@ -11,12 +11,12 @@ type PrLinkHelperSlotInput = { }; /** - * Select the reserved-height helper-slot content for the PR-link entry field. + * Select the helper-message content for the PR-link entry field. * * Priority: active message (invalid / clipboard-empty) wins over none. * No active message selects none — the input placeholder already shows the - * example URL. The slot always keeps fixed height in the UI regardless of - * which state is selected. + * example URL. The UI mounts helper text only when this is not `none` + * (conditional mount; layout may shift when a message appears or clears). */ export function selectPrLinkHelperSlotState(input: PrLinkHelperSlotInput): PrLinkHelperSlotState { if (input.message === 'invalid') { @@ -28,5 +28,18 @@ export function selectPrLinkHelperSlotState(input: PrLinkHelperSlotInput): PrLin return 'none'; } +type PrLinkClearButtonInput = { + /** Whether the uncontrolled PR-link field currently has any text. */ + readonly hasInput: boolean; +}; + +/** + * Whether the in-field clear control should render. + * Present only when the field has content; absent when empty. + */ +export function selectPrLinkClearButtonVisible(input: PrLinkClearButtonInput): boolean { + return input.hasInput; +} + export const PR_LINK_HELPER_INVALID_COPY = 'Not a GitHub pull request link'; export const PR_LINK_HELPER_CLIPBOARD_EMPTY_COPY = 'Clipboard is empty'; diff --git a/apps/mobile/src/lib/share-remote-file.test.ts b/apps/mobile/src/lib/share-remote-file.test.ts new file mode 100644 index 0000000000..dbaa42be6a --- /dev/null +++ b/apps/mobile/src/lib/share-remote-file.test.ts @@ -0,0 +1,186 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { + getSafeCacheFilename, + shareLocalFile, + shareMaterializedRemoteFile, + shareRemoteFile, + ShareRemoteFileError, +} from '@/lib/share-remote-file'; + +const expoFileSystemMock = vi.hoisted(() => { + const File = vi.fn(function FileMock() { + return {}; + }); + const Directory = vi.fn(function DirectoryMock() { + return { + create: vi.fn(), + }; + }); + return { + Directory, + File: Object.assign(File, { downloadFileAsync: vi.fn() }), + Paths: { cache: 'file:///cache' }, + }; +}); + +const reactNativeMock = vi.hoisted(() => ({ + Platform: { OS: 'ios' }, +})); + +const expoSharingMock = vi.hoisted(() => ({ + isAvailableAsync: vi.fn(), + shareAsync: vi.fn(), +})); + +vi.mock('expo-file-system', () => ({ + Directory: expoFileSystemMock.Directory, + File: expoFileSystemMock.File, + Paths: expoFileSystemMock.Paths, +})); + +vi.mock('react-native', () => ({ + Platform: reactNativeMock.Platform, +})); + +vi.mock('expo-sharing', () => ({ + isAvailableAsync: expoSharingMock.isAvailableAsync, + shareAsync: expoSharingMock.shareAsync, +})); + +beforeEach(() => { + vi.clearAllMocks(); + reactNativeMock.Platform.OS = 'ios'; + expoSharingMock.isAvailableAsync.mockResolvedValue(true); + expoSharingMock.shareAsync.mockResolvedValue(undefined); +}); + +describe('getSafeCacheFilename', () => { + it('bounds cache filenames and preserves the extension', () => { + const id = '01ARZ3NDEKTSV4RRFFQ69G5FAV'; + const filename = `${'a'.repeat(508)}.pdf`; + const cacheFilename = getSafeCacheFilename({ id, filename }); + + expect(new TextEncoder().encode(cacheFilename).byteLength).toBeLessThanOrEqual(255); + expect(cacheFilename.startsWith(`${id}-`)).toBe(true); + expect(cacheFilename.endsWith('.pdf')).toBe(true); + }); + + it('sanitizes unsafe path characters', () => { + expect(getSafeCacheFilename({ id: 'inv/1', filename: 'a b.pdf' })).toBe('inv_1-a_b.pdf'); + }); +}); + +describe('shareLocalFile', () => { + it('throws sharing-unavailable when the platform cannot share', async () => { + expoSharingMock.isAvailableAsync.mockResolvedValue(false); + + await expect(shareLocalFile('file:///tmp/a.pdf')).rejects.toMatchObject({ + reason: 'sharing-unavailable', + }); + expect(expoSharingMock.shareAsync).not.toHaveBeenCalled(); + }); + + it('presents the native share sheet when sharing is available', async () => { + await shareLocalFile('file:///tmp/a.pdf'); + expect(expoSharingMock.shareAsync).toHaveBeenCalledWith('file:///tmp/a.pdf'); + }); +}); + +describe('shareMaterializedRemoteFile', () => { + it('deletes the temp file after a successful iOS share', async () => { + const deleted: string[] = []; + await shareMaterializedRemoteFile( + { + uri: 'file:///cache/org-invoices/a.pdf', + delete: () => { + deleted.push('file:///cache/org-invoices/a.pdf'); + }, + }, + async () => { + await Promise.resolve(); + } + ); + expect(deleted).toEqual(['file:///cache/org-invoices/a.pdf']); + }); + + it('keeps the temp file after a successful Android share', async () => { + reactNativeMock.Platform.OS = 'android'; + const deleted: string[] = []; + await shareMaterializedRemoteFile( + { + uri: 'file:///cache/org-invoices/a.pdf', + delete: () => { + deleted.push('file:///cache/org-invoices/a.pdf'); + }, + }, + async () => { + await Promise.resolve(); + } + ); + expect(deleted).toEqual([]); + }); + + it('deletes the temp file after share failures', async () => { + const deleted: string[] = []; + await expect( + shareMaterializedRemoteFile( + { + uri: 'file:///cache/org-invoices/a.pdf', + delete: () => { + deleted.push('file:///cache/org-invoices/a.pdf'); + }, + }, + async () => { + await Promise.resolve(); + throw new Error('share failed'); + } + ) + ).rejects.toThrow('share failed'); + expect(deleted).toEqual(['file:///cache/org-invoices/a.pdf']); + }); +}); + +describe('shareRemoteFile', () => { + it('throws download-failed when materialization fails', async () => { + expoFileSystemMock.File.downloadFileAsync.mockRejectedValue(new Error('network down')); + + await expect( + shareRemoteFile({ + url: 'https://example.com/a.pdf', + cacheDirectoryName: 'org-invoices', + cacheKey: 'in_1', + filename: 'a.pdf', + }) + ).rejects.toBeInstanceOf(ShareRemoteFileError); + + await expect( + shareRemoteFile({ + url: 'https://example.com/a.pdf', + cacheDirectoryName: 'org-invoices', + cacheKey: 'in_1', + filename: 'a.pdf', + }) + ).rejects.toMatchObject({ reason: 'download-failed' }); + }); + + it('downloads then shares a remote file', async () => { + const downloaded = { + uri: 'file:///cache/org-invoices/in_1-a.pdf', + delete: vi.fn(), + }; + expoFileSystemMock.File.downloadFileAsync.mockResolvedValue(downloaded); + + await shareRemoteFile({ + url: 'https://example.com/a.pdf', + cacheDirectoryName: 'org-invoices', + cacheKey: 'in_1', + filename: 'a.pdf', + }); + + expect(expoFileSystemMock.Directory).toHaveBeenCalledWith('file:///cache', 'org-invoices'); + expect(expoFileSystemMock.File.downloadFileAsync).toHaveBeenCalled(); + expect(expoSharingMock.shareAsync).toHaveBeenCalledWith(downloaded.uri); + expect(downloaded.delete).toHaveBeenCalled(); + }); +}); diff --git a/apps/mobile/src/lib/share-remote-file.ts b/apps/mobile/src/lib/share-remote-file.ts new file mode 100644 index 0000000000..87c955847d --- /dev/null +++ b/apps/mobile/src/lib/share-remote-file.ts @@ -0,0 +1,186 @@ +import { Directory, File, Paths } from 'expo-file-system'; +import * as Sharing from 'expo-sharing'; +import { Platform } from 'react-native'; + +const MAX_CACHE_FILENAME_BYTES = 255; +const ONE_BYTE_CODE_POINT_MAX = 127; +const TWO_BYTE_CODE_POINT_MAX = 2047; +const THREE_BYTE_CODE_POINT_MAX = 65_535; + +export type ShareRemoteFileReason = 'sharing-unavailable' | 'download-failed'; + +export class ShareRemoteFileError extends Error { + readonly reason: ShareRemoteFileReason; + + constructor(reason: ShareRemoteFileReason) { + super(reason); + this.name = 'ShareRemoteFileError'; + this.reason = reason; + } +} + +export type MaterializedRemoteFile = { + uri: string; + delete: () => void; +}; + +export function getShareRemoteFileReason(error: unknown): ShareRemoteFileReason | null { + if (error instanceof ShareRemoteFileError) { + return error.reason; + } + return null; +} + +async function materializeRemoteFile({ + url, + cacheDirectoryName, + cacheFilename, +}: { + url: string; + cacheDirectoryName: string; + cacheFilename: string; +}): Promise { + try { + const directory = new Directory(Paths.cache, cacheDirectoryName); + directory.create({ idempotent: true, intermediates: true }); + + const file = new File(directory, cacheFilename); + const downloaded = await File.downloadFileAsync(url, file, { idempotent: true }); + return { + uri: downloaded.uri, + delete: () => { + downloaded.delete(); + }, + }; + } catch (error) { + if (error instanceof ShareRemoteFileError) { + throw error; + } + throw new ShareRemoteFileError('download-failed'); + } +} + +export async function shareLocalFile(localUri: string): Promise { + const available = await Sharing.isAvailableAsync(); + if (!available) { + throw new ShareRemoteFileError('sharing-unavailable'); + } + + await Sharing.shareAsync(localUri); +} + +export async function shareMaterializedRemoteFile( + file: MaterializedRemoteFile, + shareFile: (uri: string) => Promise = shareLocalFile +): Promise { + try { + await shareFile(file.uri); + if (Platform.OS !== 'android') { + file.delete(); + } + } catch (error) { + file.delete(); + throw error; + } +} + +export async function shareRemoteFile({ + url, + cacheDirectoryName, + cacheKey, + filename, +}: { + url: string; + cacheDirectoryName: string; + cacheKey: string; + filename: string; +}): Promise { + const materialized = await materializeRemoteFile({ + url, + cacheDirectoryName, + cacheFilename: getSafeCacheFilename({ id: cacheKey, filename }), + }); + await shareMaterializedRemoteFile(materialized); +} + +export function getSafeCacheFilename({ id, filename }: { id: string; filename: string }): string { + const prefix = `${safePathSegment(id)}-`; + const filenameBudget = MAX_CACHE_FILENAME_BYTES - utf8ByteLength(prefix); + + if (filenameBudget <= 0) { + return truncateUtf8(prefix, MAX_CACHE_FILENAME_BYTES); + } + + return `${prefix}${boundFilenameSegment(safePathSegment(filename), filenameBudget)}`; +} + +function safePathSegment(value: string): string { + const sanitized = value.trim().replaceAll(/[^a-zA-Z0-9._-]/g, '_'); + return sanitized.length > 0 ? sanitized : 'attachment'; +} + +function boundFilenameSegment(filename: string, maxBytes: number): string { + if (utf8ByteLength(filename) <= maxBytes) { + return filename; + } + + const extension = getExtension(filename); + const extensionBytes = utf8ByteLength(extension); + if (extension.length > 0 && extensionBytes < maxBytes) { + const stem = filename.slice(0, -extension.length); + const truncatedStem = truncateUtf8(stem, maxBytes - extensionBytes); + if (truncatedStem.length > 0) { + return `${truncatedStem}${extension}`; + } + } + + return truncateUtf8(filename, maxBytes); +} + +function getExtension(filename: string): string { + const extensionStart = filename.lastIndexOf('.'); + if (extensionStart <= 0 || extensionStart === filename.length - 1) { + return ''; + } + + return filename.slice(extensionStart); +} + +function utf8ByteLength(value: string): number { + let bytes = 0; + for (const character of value) { + bytes += utf8CodePointByteLength(character); + } + return bytes; +} + +function truncateUtf8(value: string, maxBytes: number): string { + let bytes = 0; + let result = ''; + + for (const character of value) { + const characterBytes = utf8CodePointByteLength(character); + if (bytes + characterBytes > maxBytes) { + break; + } + + bytes += characterBytes; + result += character; + } + + return result; +} + +function utf8CodePointByteLength(character: string): number { + const codePoint = character.codePointAt(0) ?? 0; + if (codePoint <= ONE_BYTE_CODE_POINT_MAX) { + return 1; + } + if (codePoint <= TWO_BYTE_CODE_POINT_MAX) { + return 2; + } + if (codePoint <= THREE_BYTE_CODE_POINT_MAX) { + return 3; + } + return 4; +} diff --git a/apps/web/src/lib/github-pr-review/dtos.ts b/apps/web/src/lib/github-pr-review/dtos.ts index 40d13b1618..560fa6076f 100644 --- a/apps/web/src/lib/github-pr-review/dtos.ts +++ b/apps/web/src/lib/github-pr-review/dtos.ts @@ -129,6 +129,7 @@ export const GitHubPrReviewReviewCommentSchema = z reactions: z.array(GitHubPrReviewReactionSchema), }) .strict(); +export type GitHubPrReviewReviewComment = z.infer; export const GitHubPrReviewReviewThreadSchema = z .object({ @@ -150,6 +151,7 @@ export type GitHubPrReviewReviewThread = z.infer { page: 1, hasNextPage: false, endCursor: null, + conversation: [], threads: [ { id: 'thread-1', @@ -269,6 +270,7 @@ describe('buildReviewThreadsResult', () => { expect(result.threads[0]?.line).toBeNull(); expect(result.threads[0]?.path).toBe('src/file.ts'); expect(result.threads[0]?.comments[0]?.reactions[0]?.count).toBe(2); + expect(result.conversation).toEqual([]); expect(result.nextCursor).toBeNull(); }); @@ -277,6 +279,7 @@ describe('buildReviewThreadsResult', () => { page: 1, hasNextPage: false, endCursor: null, + conversation: [], threads: [ { id: 'thread-2', @@ -304,6 +307,7 @@ describe('buildReviewThreadsResult', () => { page: 1, hasNextPage: false, endCursor: null, + conversation: [], threads: [ { id: 'thread-3', @@ -330,6 +334,7 @@ describe('buildReviewThreadsResult', () => { page: 1, hasNextPage: true, endCursor: 'Y3Vyc29yOnYyOpHOAAAAAA==', + conversation: [], threads: [], }); expect(result.nextCursor).toBe('Y3Vyc29yOnYyOpHOAAAAAA=='); @@ -342,6 +347,7 @@ describe('buildReviewThreadsResult', () => { page: 1, hasNextPage: false, endCursor: null, + conversation: [], threads: [ { id: 'thread-4', @@ -360,4 +366,33 @@ describe('buildReviewThreadsResult', () => { }); expect(result.threads[0]?.comments).toHaveLength(120); }); + + it('maps conversation comments through the same DTO shape as thread comments', () => { + const result = buildReviewThreadsResult({ + page: 1, + hasNextPage: false, + endCursor: null, + threads: [], + conversation: [ + { + databaseId: 99, + id: 'IC_99', + author: { login: 'alice', avatarUrl: 'https://avatars.example/alice' }, + body: 'top-level note', + createdAt: '2026-02-01T00:00:00Z', + reactions: [{ content: 'HEART', count: 1, viewerHasReacted: true }], + }, + ], + }); + expect(result.conversation).toEqual([ + { + commentId: 99, + nodeId: 'IC_99', + author: { login: 'alice', avatarUrl: 'https://avatars.example/alice' }, + bodyMarkdown: 'top-level note', + createdAt: '2026-02-01T00:00:00Z', + reactions: [{ content: 'HEART', count: 1, viewerHasReacted: true }], + }, + ]); + }); }); diff --git a/apps/web/src/lib/github-pr-review/mappers.ts b/apps/web/src/lib/github-pr-review/mappers.ts index 843da39184..9212c189e4 100644 --- a/apps/web/src/lib/github-pr-review/mappers.ts +++ b/apps/web/src/lib/github-pr-review/mappers.ts @@ -8,6 +8,7 @@ import { type GitHubPrReviewChecksResult, type GitHubPrReviewFile, type GitHubPrReviewFilesResult, + type GitHubPrReviewReviewComment, type GitHubPrReviewReviewThread, type GitHubPrReviewReviewThreadsResult, GitHubPrReviewFilesResultSchema, @@ -290,13 +291,32 @@ export type GraphQlReviewCommentInput = { reactions: Array<{ content: string; count: number; viewerHasReacted: boolean }>; }; +/** Map a pre-DTO GraphQL comment (normalizeComment output) into the wire DTO. */ +export function mapReviewComment(comment: GraphQlReviewCommentInput): GitHubPrReviewReviewComment { + return { + commentId: comment.databaseId, + nodeId: comment.id, + author: comment.author + ? { login: comment.author.login, avatarUrl: comment.author.avatarUrl } + : null, + bodyMarkdown: comment.body, + createdAt: comment.createdAt, + reactions: comment.reactions.map(r => ({ + content: r.content, + count: r.count, + viewerHasReacted: r.viewerHasReacted, + })), + }; +} + export function buildReviewThreadsResult(args: { threads: GraphQlReviewThreadInput[]; + conversation: GraphQlReviewCommentInput[]; page: number; hasNextPage: boolean; endCursor: string | null; }): GitHubPrReviewReviewThreadsResult { - const { threads, page, hasNextPage, endCursor } = args; + const { threads, conversation, page, hasNextPage, endCursor } = args; const dtos: GitHubPrReviewReviewThread[] = threads.map(t => { const subjectType: 'LINE' | 'FILE' = t.subjectType === 'FILE' ? 'FILE' : 'LINE'; const diffSide = t.diffSide === 'LEFT' || t.diffSide === 'RIGHT' ? t.diffSide : null; @@ -311,18 +331,7 @@ export function buildReviewThreadsResult(args: { originalLine: t.originalLine ?? null, originalStartLine: t.originalStartLine ?? null, diffSide, - comments: t.comments.map(c => ({ - commentId: c.databaseId, - nodeId: c.id, - author: c.author ? { login: c.author.login, avatarUrl: c.author.avatarUrl } : null, - bodyMarkdown: c.body, - createdAt: c.createdAt, - reactions: c.reactions.map(r => ({ - content: r.content, - count: r.count, - viewerHasReacted: r.viewerHasReacted, - })), - })), + comments: t.comments.map(mapReviewComment), }; }); const nextCursor = @@ -331,6 +340,7 @@ export function buildReviewThreadsResult(args: { : null; return GitHubPrReviewReviewThreadsResultSchema.parse({ threads: dtos, + conversation: conversation.map(mapReviewComment), nextCursor, }); } diff --git a/apps/web/src/lib/github-pr-review/review-thread-comments.test.ts b/apps/web/src/lib/github-pr-review/review-thread-comments.test.ts index cdaf2824ce..4de2045083 100644 --- a/apps/web/src/lib/github-pr-review/review-thread-comments.test.ts +++ b/apps/web/src/lib/github-pr-review/review-thread-comments.test.ts @@ -2,6 +2,7 @@ * @jest-environment node */ import { + CONVERSATION_COMMENTS_QUERY_FOR_TEST, fetchAllThreadComments, REVIEW_THREAD_COMMENTS_FOLLOWUP_QUERY_FOR_TEST, } from '@/routers/github-pr-review-router'; @@ -100,6 +101,20 @@ describe('fetchAllThreadComments', () => { expect(secondArgs.variables.after).toBe('c2'); }); + // Production GraphQL contract for top-level PR conversation comments. + // `reactors` is a connection; GitHub rejects the query without first/last. + // The local stub harness cannot catch a bare `reactors` regression. + it('locks CONVERSATION_COMMENTS_QUERY load-bearing selection shape', () => { + expect(CONVERSATION_COMMENTS_QUERY_FOR_TEST).toMatch(/query\s+PrReviewConversationComments\b/); + // Operation must select PR conversation comments (not review threads). + expect(CONVERSATION_COMMENTS_QUERY_FOR_TEST).toMatch( + /pullRequest\s*\([^)]*\)\s*\{\s*comments\s*\(/ + ); + expect(CONVERSATION_COMMENTS_QUERY_FOR_TEST).toContain('reactors(first: 0)'); + // Bare `reactors {` is invalid GraphQL against GitHub (connection needs first/last). + expect(CONVERSATION_COMMENTS_QUERY_FOR_TEST).not.toMatch(/reactors\s*\{/); + }); + it('keeps only reactionGroups with totalCount > 0 in the DTO shape', async () => { const comments = await fetchAllThreadComments({ octokit: { request: jest.fn() } as never, diff --git a/apps/web/src/routers/github-pr-review-router.test.ts b/apps/web/src/routers/github-pr-review-router.test.ts index 33c4b1a490..2997465f9d 100644 --- a/apps/web/src/routers/github-pr-review-router.test.ts +++ b/apps/web/src/routers/github-pr-review-router.test.ts @@ -183,108 +183,143 @@ describe('githubPrReviewRouter infinite-query inputs accept the tRPC direction f it('listReviewThreads accepts direction: "forward"', async () => { getGitHubUserAccessToken.mockResolvedValueOnce(connected('t1', 'auth_1', 1)); const caller = createCaller({ user: { id: 'user-1' } as User }); - buildOctokit('t1').request.mockResolvedValue({ - data: { - data: { - repository: { - pullRequest: { - reviewThreads: { nodes: [], pageInfo: { hasNextPage: false, endCursor: null } }, + // First page issues PrReviewThreads + PrReviewConversationComments in parallel. + buildOctokit('t1').request.mockImplementation( + async (_path: string, body: { query: string }) => { + if (body.query.includes('PrReviewConversationComments')) { + return { + data: { + data: { + repository: { + pullRequest: { + comments: { nodes: [], pageInfo: { hasNextPage: false, endCursor: null } }, + }, + }, + }, + }, + }; + } + return { + data: { + data: { + repository: { + pullRequest: { + reviewThreads: { nodes: [], pageInfo: { hasNextPage: false, endCursor: null } }, + }, + }, }, }, - }, - }, - }); + }; + } + ); await expect( caller.listReviewThreads({ owner: 'octocat', repo: 'hello', number: 1, direction: 'forward' }) - ).resolves.toBeDefined(); + ).resolves.toMatchObject({ threads: [], conversation: [], nextCursor: null }); }); it('listReviewThreads maps reactionGroups to non-zero DTO reactions only', async () => { getGitHubUserAccessToken.mockResolvedValueOnce(connected('t1', 'auth_1', 1)); const caller = createCaller({ user: { id: 'user-1' } as User }); - buildOctokit('t1').request.mockResolvedValue({ - data: { - data: { - repository: { - pullRequest: { - reviewThreads: { - pageInfo: { hasNextPage: false, endCursor: null }, - nodes: [ - { - id: 'PRRT_1', - isResolved: false, - isOutdated: false, - subjectType: 'LINE', - path: 'src/foo.ts', - line: 4, - startLine: null, - originalLine: 4, - originalStartLine: null, - diffSide: 'RIGHT', - comments: { - pageInfo: { hasNextPage: false, endCursor: null }, - nodes: [ - { - databaseId: 11, - id: 'PRRC_11', - body: 'nit', - createdAt: '2024-01-01T00:00:00Z', - author: { login: 'octocat', avatarUrl: 'https://x/y.png' }, - // Live schema: all group types present; zero-count filtered out. - reactionGroups: [ - { - content: 'THUMBS_UP', - viewerHasReacted: true, - reactors: { totalCount: 2 }, - }, - { - content: 'THUMBS_DOWN', - viewerHasReacted: false, - reactors: { totalCount: 0 }, - }, - { - content: 'LAUGH', - viewerHasReacted: false, - reactors: { totalCount: 0 }, - }, - { - content: 'HOORAY', - viewerHasReacted: false, - reactors: { totalCount: 0 }, - }, - { - content: 'CONFUSED', - viewerHasReacted: false, - reactors: { totalCount: 0 }, - }, - { - content: 'HEART', - viewerHasReacted: false, - reactors: { totalCount: 1 }, - }, - { - content: 'ROCKET', - viewerHasReacted: false, - reactors: { totalCount: 0 }, - }, + buildOctokit('t1').request.mockImplementation( + async (_path: string, body: { query: string }) => { + if (body.query.includes('PrReviewConversationComments')) { + return { + data: { + data: { + repository: { + pullRequest: { + comments: { nodes: [], pageInfo: { hasNextPage: false, endCursor: null } }, + }, + }, + }, + }, + }; + } + return { + data: { + data: { + repository: { + pullRequest: { + reviewThreads: { + pageInfo: { hasNextPage: false, endCursor: null }, + nodes: [ + { + id: 'PRRT_1', + isResolved: false, + isOutdated: false, + subjectType: 'LINE', + path: 'src/foo.ts', + line: 4, + startLine: null, + originalLine: 4, + originalStartLine: null, + diffSide: 'RIGHT', + comments: { + pageInfo: { hasNextPage: false, endCursor: null }, + nodes: [ { - content: 'EYES', - viewerHasReacted: false, - reactors: { totalCount: 0 }, + databaseId: 11, + id: 'PRRC_11', + body: 'nit', + createdAt: '2024-01-01T00:00:00Z', + author: { login: 'octocat', avatarUrl: 'https://x/y.png' }, + // Live schema: all group types present; zero-count filtered out. + reactionGroups: [ + { + content: 'THUMBS_UP', + viewerHasReacted: true, + reactors: { totalCount: 2 }, + }, + { + content: 'THUMBS_DOWN', + viewerHasReacted: false, + reactors: { totalCount: 0 }, + }, + { + content: 'LAUGH', + viewerHasReacted: false, + reactors: { totalCount: 0 }, + }, + { + content: 'HOORAY', + viewerHasReacted: false, + reactors: { totalCount: 0 }, + }, + { + content: 'CONFUSED', + viewerHasReacted: false, + reactors: { totalCount: 0 }, + }, + { + content: 'HEART', + viewerHasReacted: false, + reactors: { totalCount: 1 }, + }, + { + content: 'ROCKET', + viewerHasReacted: false, + reactors: { totalCount: 0 }, + }, + { + content: 'EYES', + viewerHasReacted: false, + reactors: { totalCount: 0 }, + }, + ], }, ], }, - ], - }, + }, + ], }, - ], + }, }, }, }, - }, - }, - }); + }; + } + ); const result = await caller.listReviewThreads({ owner: 'octocat', @@ -294,6 +329,7 @@ describe('githubPrReviewRouter infinite-query inputs accept the tRPC direction f }); expect(result.threads).toHaveLength(1); + expect(result.conversation).toEqual([]); expect(result.threads[0]?.comments[0]?.reactions).toEqual([ { content: 'THUMBS_UP', count: 2, viewerHasReacted: true }, { content: 'HEART', count: 1, viewerHasReacted: false }, @@ -301,6 +337,258 @@ describe('githubPrReviewRouter infinite-query inputs accept the tRPC direction f }); }); +describe('githubPrReviewRouter.listReviewThreads conversation comments', () => { + const emptyThreads = { + nodes: [] as unknown[], + pageInfo: { hasNextPage: false, endCursor: null as string | null }, + }; + + function conversationNode(overrides: { + databaseId: number; + id: string; + body: string; + createdAt?: string; + }) { + return { + databaseId: overrides.databaseId, + id: overrides.id, + body: overrides.body, + createdAt: overrides.createdAt ?? '2026-01-01T00:00:00Z', + author: { login: 'alice', avatarUrl: 'https://avatars.example/alice' }, + reactionGroups: [ + { content: 'THUMBS_UP', viewerHasReacted: false, reactors: { totalCount: 1 } }, + { content: 'HEART', viewerHasReacted: false, reactors: { totalCount: 0 } }, + ], + }; + } + + function mockGraphqlByOperation( + octokit: OctokitMock, + handlers: { + threads?: (vars: Record) => unknown; + conversation?: (vars: Record) => unknown; + } + ) { + octokit.request.mockImplementation( + async (_path: string, body: { query: string; variables: Record }) => { + if (body.query.includes('query PrReviewConversationComments')) { + const payload = handlers.conversation?.(body.variables) ?? { + nodes: [], + pageInfo: { hasNextPage: false, endCursor: null }, + }; + return { + data: { + data: { repository: { pullRequest: { comments: payload } } }, + }, + }; + } + if (body.query.includes('query PrReviewThreads')) { + const payload = handlers.threads?.(body.variables) ?? emptyThreads; + return { + data: { + data: { repository: { pullRequest: { reviewThreads: payload } } }, + }, + }; + } + throw new Error(`unexpected GraphQL operation: ${body.query.slice(0, 80)}`); + } + ); + } + + it('returns mapped conversation comments on the first page', async () => { + getGitHubUserAccessToken.mockResolvedValueOnce(connected('t1', 'auth_1', 1)); + const caller = createCaller({ user: { id: 'user-1' } as User }); + mockGraphqlByOperation(buildOctokit('t1'), { + conversation: () => ({ + nodes: [conversationNode({ databaseId: 42, id: 'IC_42', body: 'top-level' })], + pageInfo: { hasNextPage: false, endCursor: null }, + }), + }); + + const result = await caller.listReviewThreads({ + owner: 'octocat', + repo: 'hello', + number: 1, + }); + + expect(result.conversation).toEqual([ + { + commentId: 42, + nodeId: 'IC_42', + author: { login: 'alice', avatarUrl: 'https://avatars.example/alice' }, + bodyMarkdown: 'top-level', + createdAt: '2026-01-01T00:00:00Z', + reactions: [{ content: 'THUMBS_UP', count: 1, viewerHasReacted: false }], + }, + ]); + expect(result.threads).toEqual([]); + }); + + it('returns conversation: [] when the PR has no conversation comments', async () => { + getGitHubUserAccessToken.mockResolvedValueOnce(connected('t1', 'auth_1', 1)); + const caller = createCaller({ user: { id: 'user-1' } as User }); + mockGraphqlByOperation(buildOctokit('t1'), { + conversation: () => ({ + nodes: [], + pageInfo: { hasNextPage: false, endCursor: null }, + }), + }); + + const result = await caller.listReviewThreads({ + owner: 'octocat', + repo: 'hello', + number: 1, + }); + + expect(result.conversation).toEqual([]); + }); + + it('paginates conversation comments to completion across multiple pages', async () => { + getGitHubUserAccessToken.mockResolvedValueOnce(connected('t1', 'auth_1', 1)); + const caller = createCaller({ user: { id: 'user-1' } as User }); + let conversationCalls = 0; + mockGraphqlByOperation(buildOctokit('t1'), { + conversation: vars => { + conversationCalls += 1; + if (vars.after == null) { + return { + nodes: [conversationNode({ databaseId: 1, id: 'IC_1', body: 'page-1' })], + pageInfo: { hasNextPage: true, endCursor: 'cursor-1' }, + }; + } + if (vars.after === 'cursor-1') { + return { + nodes: [conversationNode({ databaseId: 2, id: 'IC_2', body: 'page-2' })], + pageInfo: { hasNextPage: false, endCursor: 'cursor-2' }, + }; + } + throw new Error(`unexpected after cursor: ${String(vars.after)}`); + }, + }); + + const result = await caller.listReviewThreads({ + owner: 'octocat', + repo: 'hello', + number: 1, + }); + + expect(conversationCalls).toBe(2); + expect(result.conversation.map((c: { commentId: number }) => c.commentId)).toEqual([1, 2]); + }); + + it('truncates conversation comments after CONVERSATION_COMMENTS_MAX_PAGES', async () => { + getGitHubUserAccessToken.mockResolvedValueOnce(connected('t1', 'auth_1', 1)); + const caller = createCaller({ user: { id: 'user-1' } as User }); + let conversationCalls = 0; + mockGraphqlByOperation(buildOctokit('t1'), { + conversation: () => { + conversationCalls += 1; + // Always report another page so the loop hits the hard cap. + const pageIndex = conversationCalls; + return { + nodes: [ + conversationNode({ + databaseId: pageIndex, + id: `IC_${pageIndex}`, + body: `page-${pageIndex}`, + }), + ], + pageInfo: { + hasNextPage: true, + endCursor: `cursor-${pageIndex}`, + }, + }; + }, + }); + + const result = await caller.listReviewThreads({ + owner: 'octocat', + repo: 'hello', + number: 1, + }); + + // Cap is 5 pages (CONVERSATION_COMMENTS_MAX_PAGES); further pages are dropped. + expect(conversationCalls).toBe(5); + expect(result.conversation).toHaveLength(5); + expect(result.conversation.map((c: { commentId: number }) => c.commentId)).toEqual([ + 1, 2, 3, 4, 5, + ]); + }); + + it('returns conversation: [] on a cursored page and does not issue PrReviewConversationComments', async () => { + getGitHubUserAccessToken.mockResolvedValueOnce(connected('t1', 'auth_1', 1)); + const caller = createCaller({ user: { id: 'user-1' } as User }); + const octokit = buildOctokit('t1'); + let conversationCalls = 0; + mockGraphqlByOperation(octokit, { + conversation: () => { + conversationCalls += 1; + return { + nodes: [conversationNode({ databaseId: 99, id: 'IC_99', body: 'should-not-fetch' })], + pageInfo: { hasNextPage: false, endCursor: null }, + }; + }, + threads: () => ({ + nodes: [], + pageInfo: { hasNextPage: false, endCursor: null }, + }), + }); + + const result = await caller.listReviewThreads({ + owner: 'octocat', + repo: 'hello', + number: 1, + cursor: 'Y3Vyc29yOnYyOpHOAAAAAA==', + }); + + expect(result.conversation).toEqual([]); + expect(conversationCalls).toBe(0); + const queries = octokit.request.mock.calls.map( + (call: unknown[]) => (call[1] as { query: string }).query + ); + expect(queries.some((q: string) => q.includes('PrReviewConversationComments'))).toBe(false); + expect(queries.some((q: string) => q.includes('PrReviewThreads'))).toBe(true); + }); + + it('null-connection return shape includes conversation: [] via the builder', async () => { + getGitHubUserAccessToken.mockResolvedValueOnce(connected('t1', 'auth_1', 1)); + const caller = createCaller({ user: { id: 'user-1' } as User }); + const octokit = buildOctokit('t1'); + // Null pullRequest on PrReviewThreads → early null-connection path through the builder. + octokit.request.mockImplementation(async (_path: string, body: { query: string }) => { + if (body.query.includes('query PrReviewConversationComments')) { + return { + data: { + data: { + repository: { + pullRequest: { + comments: { nodes: [], pageInfo: { hasNextPage: false, endCursor: null } }, + }, + }, + }, + }, + }; + } + if (body.query.includes('query PrReviewThreads')) { + return { + data: { + data: { repository: { pullRequest: null } }, + }, + }; + } + throw new Error(`unexpected GraphQL operation: ${body.query.slice(0, 80)}`); + }); + + const result = await caller.listReviewThreads({ + owner: 'octocat', + repo: 'hello', + number: 1, + }); + + expect(result).toEqual({ threads: [], conversation: [], nextCursor: null }); + }); +}); + describe('githubPrReviewRouter mutations go through withGitHubUserTokenRetry', () => { it('rotates the credential and retries on a raw 401', async () => { getGitHubUserAccessToken diff --git a/apps/web/src/routers/github-pr-review-router.ts b/apps/web/src/routers/github-pr-review-router.ts index 6c11055234..68f0bf973f 100644 --- a/apps/web/src/routers/github-pr-review-router.ts +++ b/apps/web/src/routers/github-pr-review-router.ts @@ -13,6 +13,8 @@ import { sliceFileLines, } from '@/lib/github-pr-review/mappers'; import { + CONVERSATION_COMMENTS_MAX_PAGES, + CONVERSATION_COMMENTS_PAGE_SIZE, FILE_LINES_MAX, FILES_MAX_PAGES, FILES_PAGE_SIZE, @@ -274,6 +276,48 @@ const REVIEW_THREAD_COMMENTS_FOLLOWUP_QUERY = /* GraphQL */ ` } `; +// PR conversation (issue) comments — separate from reviewThreads so this +// connection can be paginated to completion on the first listReviewThreads +// page only. Node selection matches the live review-comment selection so +// normalizeComment / normalizeReactions apply unchanged. +const CONVERSATION_COMMENTS_QUERY = /* GraphQL */ ` + query PrReviewConversationComments( + $owner: String! + $name: String! + $number: Int! + $first: Int! + $after: String + ) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { + comments(first: $first, after: $after) { + pageInfo { + hasNextPage + endCursor + } + nodes { + databaseId + id + body + createdAt + author { + login + avatarUrl + } + reactionGroups { + content + viewerHasReacted + reactors(first: 0) { + totalCount + } + } + } + } + } + } + } +`; + const ENABLE_AUTO_MERGE_MUTATION = /* GraphQL */ ` mutation EnableAutoMerge($input: EnablePullRequestAutoMergeInput!) { enablePullRequestAutoMerge(input: $input) { @@ -395,6 +439,7 @@ function normalizeComment(node: GraphQlCommentNode) { // Exported for unit testing the follow-up pagination loop. export const REVIEW_THREAD_COMMENTS_FOLLOWUP_QUERY_FOR_TEST = REVIEW_THREAD_COMMENTS_FOLLOWUP_QUERY; +export const CONVERSATION_COMMENTS_QUERY_FOR_TEST = CONVERSATION_COMMENTS_QUERY; export async function fetchAllThreadComments(args: { octokit: ReturnType; @@ -428,6 +473,71 @@ export async function fetchAllThreadComments(args: { return collected; } +async function fetchConversationCommentsPage(args: { + octokit: ReturnType; + owner: string; + repo: string; + number: number; + cursor: string | null; +}): Promise { + const { octokit, owner, repo, number, cursor } = args; + const response = (await octokit.request('POST /graphql', { + query: CONVERSATION_COMMENTS_QUERY, + variables: { + owner, + name: repo, + number, + first: CONVERSATION_COMMENTS_PAGE_SIZE, + after: cursor ?? null, + }, + })) as { + data: { + data: { + repository: { + pullRequest: { + comments: GraphQlCommentConnection; + } | null; + } | null; + } | null; + errors?: unknown; + }; + }; + throwTrpcFromGraphQlErrors(response.data.errors as never); + return response.data.data?.repository?.pullRequest?.comments ?? null; +} + +// First-page-only conversation comments. Loops against pageInfo up to +// CONVERSATION_COMMENTS_MAX_PAGES × CONVERSATION_COMMENTS_PAGE_SIZE (5 × 100). +// Past the cap, remaining pages are dropped and whatever was collected is +// returned (silent truncation) — same ceiling spirit as bot review-comment +// pagination (bot/platforms/github.ts). +export async function fetchAllConversationComments(args: { + octokit: ReturnType; + owner: string; + repo: string; + number: number; +}): Promise[]> { + const { octokit, owner, repo, number } = args; + const collected: ReturnType[] = []; + let cursor: string | null = null; + for (let page = 1; page <= CONVERSATION_COMMENTS_MAX_PAGES; page += 1) { + const connection = await fetchConversationCommentsPage({ + octokit, + owner, + repo, + number, + cursor, + }); + if (!connection) break; + collected.push(...connection.nodes.map(normalizeComment)); + if (!connection.pageInfo.hasNextPage || !connection.pageInfo.endCursor) { + return collected; + } + cursor = connection.pageInfo.endCursor; + } + return collected; +} + async function fetchReviewThreadsPage(args: { octokit: ReturnType; owner: string; @@ -640,15 +750,33 @@ export const githubPrReviewRouter = createTRPCRouter({ return withGitHubUserTokenRetry({ kiloUserId: ctx.user.id, call: async octokit => { - const connection = await fetchReviewThreadsPage({ - octokit, - owner: input.owner, - repo: input.repo, - number: input.number, - cursor: input.cursor ?? null, - }); + const isFirstPage = input.cursor == null; + const [connection, conversation] = await Promise.all([ + fetchReviewThreadsPage({ + octokit, + owner: input.owner, + repo: input.repo, + number: input.number, + cursor: input.cursor ?? null, + }), + // Conversation comments only on the first page; cursored pages get []. + isFirstPage + ? fetchAllConversationComments({ + octokit, + owner: input.owner, + repo: input.repo, + number: input.number, + }) + : Promise.resolve([]), + ]); if (!connection) { - return { threads: [], nextCursor: null }; + return buildReviewThreadsResult({ + threads: [], + conversation, + page: 1, + hasNextPage: false, + endCursor: null, + }); } const threads = await Promise.all( connection.nodes.map(async node => { @@ -674,6 +802,7 @@ export const githubPrReviewRouter = createTRPCRouter({ ); return buildReviewThreadsResult({ threads: threads as never, + conversation, page: 1, hasNextPage: connection.pageInfo.hasNextPage, endCursor: connection.pageInfo.endCursor,