From 2635aaf5841b859718086f2ebe359dda6f3f3e40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Sun, 26 Jul 2026 07:14:55 +0200 Subject: [PATCH 01/12] Add local GitHub API stub for PR review E2E --- apps/mobile/e2e/AGENTS.md | 16 +- apps/mobile/e2e/github-api-stub/server.mjs | 445 +++++++++++++++++++++ 2 files changed, 459 insertions(+), 2 deletions(-) create mode 100644 apps/mobile/e2e/github-api-stub/server.mjs diff --git a/apps/mobile/e2e/AGENTS.md b/apps/mobile/e2e/AGENTS.md index 9f7db57102..9a7414da20 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 @@ -99,7 +111,7 @@ apps/mobile/e2e/login.sh [email] # default: e2e-mobile+ ``` -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 uses a plus-tag per worktree (`e2e-mobile+@example.com`), but `normalizeEmail` in `apps/web/src/lib/utils.ts` strips plus-tags, so every worktree's default login resolves to one shared backend user (`e2e-mobile@example.com`). Seeded token rows and any rows inserted into shared tables are therefore visible across worktrees; assertions must target run-unique values, never list length, emptiness, or position. 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. 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), + }) + ); +}); From fbbf097b648d9dc498cac0a9413991ead468aa2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Sun, 26 Jul 2026 07:15:11 +0200 Subject: [PATCH 02/12] Include conversation comments in the PR review discussion payload --- .../review-discussion-reducers.test.ts | 6 +- apps/web/src/lib/github-pr-review/dtos.ts | 8 + .../src/lib/github-pr-review/mappers.test.ts | 35 ++ apps/web/src/lib/github-pr-review/mappers.ts | 36 +- .../review-thread-comments.test.ts | 15 + .../routers/github-pr-review-router.test.ts | 456 ++++++++++++++---- .../src/routers/github-pr-review-router.ts | 145 +++++- 7 files changed, 593 insertions(+), 108 deletions(-) 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/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..204c172e9e 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: vars => { + 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, From 22011d98e45405f3506c6953d34ee8967ec282f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Sun, 26 Jul 2026 07:15:12 +0200 Subject: [PATCH 03/12] Merge conversation comments into the PR discussion list --- .../pr-review/pr-review-discussion-tab.tsx | 117 +++++------- .../review-discussion-merge.test.ts | 180 ++++++++++++++++++ .../review-discussion-types.test.ts | 20 -- .../discussion/review-discussion-types.ts | 113 +++++++++-- .../use-pr-review-discussion-threads.ts | 12 +- 5 files changed, 335 insertions(+), 107 deletions(-) create mode 100644 apps/mobile/src/lib/pr-review/discussion/review-discussion-merge.test.ts diff --git a/apps/mobile/src/components/pr-review/pr-review-discussion-tab.tsx b/apps/mobile/src/components/pr-review/pr-review-discussion-tab.tsx index 1b83131c86..0194f6b6dc 100644 --- a/apps/mobile/src/components/pr-review/pr-review-discussion-tab.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-discussion-tab.tsx @@ -1,8 +1,11 @@ // PR review Discussion tab body. // -// State matrix (per S7b §6 Discussion): -// - happy: threads render, grouped by file path; first -// page auto-loads, "Load more" paginates. +// State matrix (per S7b §6 Discussion + Batch F item 2): +// - happy: one merged ascending list of review threads and +// conversation comments; first page auto-loads, +// "Load more" paginates threads. Full re-sort of +// the entire loaded set on every update (R4: a +// later page can insert rows mid-list). // - loading: first page in flight; render `Skeleton` // placeholders matching the row dimensions. // - retryable: first page failed with a transient error; @@ -19,17 +22,15 @@ // connect gate (the connect flow is owned by the // screen-level `PrReviewConnectGate`, which is // already mounted by the parent screen). -// - empty: first page returned zero threads AND no -// terminal error; render `EmptyState` with the -// "No review comments yet" copy and a "Review -// files" CTA that switches to the Files tab via -// the `onRequestFiles` prop (the screen must -// pass it; we degrade gracefully if it's -// omitted). +// - empty: first page returned zero threads AND zero +// conversation comments AND no terminal error; +// render `EmptyState` with copy covering both +// kinds and a "Review files" CTA that switches +// to the Files tab via `onRequestFiles`. // // - later-page error: a per-page refetch failure during a // "Load more" tap. The current loaded -// threads are kept and a small retry row +// items are kept and a small retry row // renders at the bottom of the list. // // The component does NOT own a ScrollView — the tab is mounted @@ -41,6 +42,7 @@ import { FlashList } from '@shopify/flash-list'; import { MessageSquarePlus } from 'lucide-react-native'; import { View } from 'react-native'; +import { CommentRow } from '@/components/pr-review/discussion/comment-row'; import { DiscussionThread } from '@/components/pr-review/discussion/discussion-thread'; import { PrReviewReconnectNotice } from '@/components/pr-review/pr-review-reconnect-notice'; import { EmptyState } from '@/components/empty-state'; @@ -49,25 +51,28 @@ import { Button } from '@/components/ui/button'; import { Skeleton } from '@/components/ui/skeleton'; import { Text } from '@/components/ui/text'; import { - groupThreadsByPath, - type ReviewThread, + type DiscussionListItem, + isDiscussionEmpty, + mergeDiscussionListItems, } from '@/lib/pr-review/discussion/review-discussion-types'; import { usePrReviewDiscussionThreads } from '@/lib/pr-review/discussion/use-pr-review-discussion-threads'; -import { cn } from '@/lib/utils'; type PrReviewDiscussionTabProps = { readonly owner: string; readonly repo: string; readonly number: number; /** - * Invoked by the empty state ("No review comments yet") to switch - * to the Files tab. Optional; if absent, the CTA is hidden. + * Invoked by the empty state to switch to the Files tab. + * Optional; if absent, the CTA is hidden. */ readonly onRequestFiles?: () => void; }; const SKELETON_ROW_COUNT = 4; const DISCUSSION_LIST_CONTENT_STYLE = { paddingTop: 12 }; +const noopReactionToggle = () => { + // Conversation comments are read-only (A2.3): no reaction mutations. +}; export function PrReviewDiscussionTab({ owner, @@ -75,11 +80,12 @@ export function PrReviewDiscussionTab({ number, onRequestFiles, }: PrReviewDiscussionTabProps) { - const { query, threads, firstPageErrorState, laterPageError } = usePrReviewDiscussionThreads({ - owner, - repo, - number, - }); + const { query, threads, conversation, firstPageErrorState, laterPageError } = + usePrReviewDiscussionThreads({ + owner, + repo, + number, + }); // ── First-page error / terminal states ───────────────────────────── if (firstPageErrorState) { @@ -139,14 +145,14 @@ export function PrReviewDiscussionTab({ ); } - // ── Empty ────────────────────────────────────────────────────────── - if (threads.length === 0) { + // ── Empty (neither threads nor conversation comments) ────────────── + if (isDiscussionEmpty(threads, conversation)) { return ( @@ -160,28 +166,28 @@ export function PrReviewDiscussionTab({ } // ── Happy / paginated list ───────────────────────────────────────── - const groups = groupThreadsByPath(threads); - // Flatten the grouped list into a single list with separator - // rows between groups. Separator rows have `type: 'separator'` - // and the threads have `type: 'thread'`. - const listItems: ListItem[] = []; - for (const group of groups) { - if (groups.length > 1) { - listItems.push({ type: 'separator', path: group.path }); - } - for (const thread of group.threads) { - listItems.push({ type: 'thread', thread }); - } - } + // Full re-sort of every loaded thread + first-page conversation + // comments (R4: "Load more" may insert rows mid-list; accepted). + const listItems = mergeDiscussionListItems(threads, conversation); return ( item.type} + getItemType={item => item.kind} renderItem={({ item }) => { - if (item.type === 'separator') { - return ; + if (item.kind === 'comment') { + return ( + + + + + + ); } return ( @@ -209,31 +215,10 @@ export function PrReviewDiscussionTab({ ); } -// ── List item shape ────────────────────────────────────────────────── - -type ListItem = - | { readonly type: 'separator'; readonly path: string } - | { - readonly type: 'thread'; - readonly thread: ReviewThread; - }; - -function keyForItem(item: ListItem): string { - return item.type === 'separator' ? `sep:${item.path}` : `thread:${item.thread.threadId}`; -} - -function GroupSeparator({ path }: Readonly<{ path: string }>) { - return ( - - - {path} - - - - ); +function keyForItem(item: DiscussionListItem): string { + return item.kind === 'thread' + ? `thread:${item.thread.threadId}` + : `comment:${item.comment.nodeId}`; } // ── Footer (Load more / error row) ─────────────────────────────────── 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-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, }; From 2081047f1952e2d4dc16a78729c7b48c1d929f8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Sun, 26 Jul 2026 07:15:26 +0200 Subject: [PATCH 04/12] Open GitHub PRs in the app from a code review --- .../code-reviewer/review-detail-screen.tsx | 16 ++++++ .../code-reviewer-open-pr-destination.test.ts | 52 +++++++++++++++++++ .../lib/code-reviewer-open-pr-destination.ts | 26 ++++++++++ 3 files changed, 94 insertions(+) create mode 100644 apps/mobile/src/lib/code-reviewer-open-pr-destination.test.ts create mode 100644 apps/mobile/src/lib/code-reviewer-open-pr-destination.ts 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/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'; From 2dbe14f86aa404454533088389f3679f21be1360 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Sun, 26 Jul 2026 08:35:35 +0200 Subject: [PATCH 07/12] Size the PR link clear control to a true 44pt target --- .../src/components/pr-review/pr-review-entry-screen.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/apps/mobile/src/components/pr-review/pr-review-entry-screen.tsx b/apps/mobile/src/components/pr-review/pr-review-entry-screen.tsx index 356552518d..db925ee831 100644 --- a/apps/mobile/src/components/pr-review/pr-review-entry-screen.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-entry-screen.tsx @@ -253,6 +253,8 @@ export function PrReviewEntryScreen() { }} /> {showClearButton ? ( + // h-13 w-13 measures 45×45pt on device; h-12 is 42pt and h-11 is + // 38pt in this app — do not "simplify" back to h-11/w-11. { applyFieldText(''); @@ -260,7 +262,7 @@ export function PrReviewEntryScreen() { }} accessibilityRole="button" accessibilityLabel="Clear pull request link" - className="h-11 w-11 items-center justify-center active:opacity-70" + className="h-13 w-13 items-center justify-center active:opacity-70" > From bb531ae9d16c595dda75eeb020a88d5b495f1ca5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Sun, 26 Jul 2026 09:29:21 +0200 Subject: [PATCH 08/12] Clear the PR link field with TextInput.clear() on iOS --- .../components/pr-review/pr-review-entry-screen.tsx | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/apps/mobile/src/components/pr-review/pr-review-entry-screen.tsx b/apps/mobile/src/components/pr-review/pr-review-entry-screen.tsx index db925ee831..c23d2140a5 100644 --- a/apps/mobile/src/components/pr-review/pr-review-entry-screen.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-entry-screen.tsx @@ -257,7 +257,15 @@ export function PrReviewEntryScreen() { // 38pt in this app — do not "simplify" back to h-11/w-11. { - applyFieldText(''); + // clear() is the iOS-safe native empty after real typing. + // setNativeProps({ text: '' }) loses the most-recent-event-count + // race and leaves the typed text visible while React state + // thinks the field is empty. Do not route through + // applyFieldText('') (paste-only path) and do not push an + // echo for '' — a non-arriving echo would stale the FIFO. + inputValueRef.current = ''; + setHasInput(false); + inputRef.current?.clear(); inputRef.current?.focus(); }} accessibilityRole="button" From 89bdb13544cd9644ff2ff3d22f3f71978bec3121 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Sun, 26 Jul 2026 09:58:58 +0200 Subject: [PATCH 09/12] Fix unused parameter in the conversation pagination test --- apps/web/src/routers/github-pr-review-router.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 204c172e9e..2997465f9d 100644 --- a/apps/web/src/routers/github-pr-review-router.test.ts +++ b/apps/web/src/routers/github-pr-review-router.test.ts @@ -481,7 +481,7 @@ describe('githubPrReviewRouter.listReviewThreads conversation comments', () => { const caller = createCaller({ user: { id: 'user-1' } as User }); let conversationCalls = 0; mockGraphqlByOperation(buildOctokit('t1'), { - conversation: vars => { + conversation: () => { conversationCalls += 1; // Always report another page so the loop hits the hard cap. const pageIndex = conversationCalls; From 664dac3acb1d23de713ad62f681515a1f70f2280 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Sun, 26 Jul 2026 10:21:53 +0200 Subject: [PATCH 10/12] Retrigger review checks From 1df59c3c5d43821216dc1ec9b9fe28ee757bb287 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Sun, 26 Jul 2026 12:39:58 +0200 Subject: [PATCH 11/12] Use hyphen-separated per-worktree E2E sign-in accounts The plus-addressed default (e2e-mobile+@example.com) collapsed to one shared backend user because normalizeEmail strips plus aliases, so concurrent worktrees signed into the same account. Derive e2e-mobile-@example.com instead; hyphens survive normalization and each worktree gets a distinct user. Lowercase the derived local part so first-time signups pass validateMagicLinkSignupEmail. --- apps/mobile/e2e/AGENTS.md | 6 +++--- apps/mobile/e2e/login.sh | 6 +++--- apps/mobile/e2e/remote-cli.sh | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/apps/mobile/e2e/AGENTS.md b/apps/mobile/e2e/AGENTS.md index 9a7414da20..b6ef20bda7 100644 --- a/apps/mobile/e2e/AGENTS.md +++ b/apps/mobile/e2e/AGENTS.md @@ -107,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 uses a plus-tag per worktree (`e2e-mobile+@example.com`), but `normalizeEmail` in `apps/web/src/lib/utils.ts` strips plus-tags, so every worktree's default login resolves to one shared backend user (`e2e-mobile@example.com`). Seeded token rows and any rows inserted into shared tables are therefore visible across worktrees; assertions must target run-unique values, never list length, emptiness, or position. 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. @@ -175,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/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" From 4a3610f0a6fb31d56af9585d4de15301a90749fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Sun, 26 Jul 2026 13:10:14 +0200 Subject: [PATCH 12/12] Retrigger review