diff --git a/package-lock.json b/package-lock.json index 24a3e9d..a55a67b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8452,7 +8452,7 @@ }, "packages/cli": { "name": "diffity", - "version": "0.9.10", + "version": "0.9.13", "license": "MIT", "dependencies": { "commander": "^14.0.3", @@ -8476,7 +8476,7 @@ }, "packages/git": { "name": "@diffity/git", - "version": "0.9.10", + "version": "0.9.13", "devDependencies": { "@types/node": "^25.5.0", "typescript": "^5.9.3", @@ -8485,7 +8485,7 @@ }, "packages/github": { "name": "@diffity/github", - "version": "0.9.10", + "version": "0.9.13", "dependencies": { "@diffity/parser": "*" }, @@ -8497,7 +8497,7 @@ }, "packages/parser": { "name": "@diffity/parser", - "version": "0.9.10", + "version": "0.9.13", "devDependencies": { "typescript": "^5.9.3", "vitest": "^4.1.0" @@ -8505,7 +8505,7 @@ }, "packages/ui": { "name": "@diffity/ui", - "version": "0.9.10", + "version": "0.9.13", "dependencies": { "@react-router/node": "^7.13.2", "@tailwindcss/vite": "^4.2.1", diff --git a/package.json b/package.json index 9a06edc..600f756 100644 --- a/package.json +++ b/package.json @@ -14,12 +14,13 @@ "scripts": { "build": "tsx scripts/build.ts", "build:skills": "tsx scripts/build-skills.ts", - "test": "npm run test -w @diffity/git && npm run test -w @diffity/github && npm run test -w @diffity/parser && npm run test -w @diffity/ui && npm run test -w diffity && npm run test:scripts", + "test": "npm run typecheck && npm run test -w @diffity/git && npm run test -w @diffity/github && npm run test -w @diffity/parser && npm run test -w @diffity/ui && npm run test -w diffity && npm run test:scripts", "link-dev": "tsx scripts/link-dev.ts", "dev": "tsx scripts/dev.ts", "release:patch": "npm run build && tsx scripts/release.ts patch && npm publish -w packages/cli", "release:minor": "npm run build && tsx scripts/release.ts minor && npm publish -w packages/cli", - "test:scripts": "vitest run scripts" + "test:scripts": "vitest run scripts", + "typecheck": "npm run typecheck -w @diffity/ui && npm run typecheck -w diffity" }, "keywords": [ "git", diff --git a/packages/cli/package.json b/packages/cli/package.json index 51737a8..90e91ca 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -1,6 +1,6 @@ { "name": "diffity", - "version": "0.9.10", + "version": "0.9.13", "description": "GitHub-style git diff viewer in the browser", "type": "module", "bin": { @@ -11,7 +11,8 @@ "dev": "tsx src/index.ts", "dev:watch": "tsx build.ts --watch", "test": "vitest run", - "test:watch": "vitest" + "test:watch": "vitest", + "typecheck": "tsc --noEmit" }, "dependencies": { "commander": "^14.0.3", diff --git a/packages/cli/src/agent.ts b/packages/cli/src/agent.ts index 01d3668..408de2e 100644 --- a/packages/cli/src/agent.ts +++ b/packages/cli/src/agent.ts @@ -22,6 +22,7 @@ import { createHash } from 'node:crypto'; import { createTour, addTourStep, updateTourStatus, deleteTour, deleteToursForSession, getTour } from './tours.js'; import { unansweredRequest } from './live-unanswered.js'; import { readAnchor, clampToFile, countWorkingTreeLines } from './anchor.js'; +import { unescapeMarkdown as fromShell } from './unescape.js'; import { startReviewRun, finishReviewRun } from './review-run.js'; import { readRepoConfig, DEFAULT_SEVERITIES, resolveInRepo, REPO_CONFIG_FILE } from '@diffity/git'; import { readFileSync } from 'node:fs'; @@ -251,7 +252,7 @@ Examples: opts.side, startLine, endLine, - opts.body, + fromShell(opts.body), { name: 'Agent', type: 'agent' }, // Recorded so the finding can follow its code when a later commit moves it. opts.side === 'new' ? readAnchor(opts.file, startLine, endLine) : undefined, @@ -268,7 +269,7 @@ Examples: const session = requireSession(); const thread = resolveThreadId(id, session.id); const author = opts.summary ? { name: 'Agent', type: 'agent' as const } : undefined; - updateThreadStatus(thread.id, 'resolved', opts.summary, author); + updateThreadStatus(thread.id, 'resolved', fromShell(opts.summary ?? ''), author); console.log(pc.green(`Resolved thread ${thread.id.slice(0, 8)}`)); }); @@ -281,7 +282,7 @@ Examples: const session = requireSession(); const thread = resolveThreadId(id, session.id); const author = opts.reason ? { name: 'Agent', type: 'agent' as const } : undefined; - updateThreadStatus(thread.id, 'dismissed', opts.reason, author); + updateThreadStatus(thread.id, 'dismissed', fromShell(opts.reason ?? ''), author); console.log(pc.green(`Dismissed thread ${thread.id.slice(0, 8)}`)); }); @@ -296,7 +297,7 @@ Examples: const session = requireSession(); const thread = resolveThreadId(id, session.id); const stillOpen = unansweredRequest(thread.comments); - addReply(thread.id, opts.body, { name: 'Agent', type: 'agent' }, opts.aside ? 'aside' : 'review'); + addReply(thread.id, fromShell(opts.body), { name: 'Agent', type: 'agent' }, opts.aside ? 'aside' : 'review'); if (opts.answers && !answerLiveRequest(opts.answers)) { console.error( pc.yellow( @@ -424,7 +425,7 @@ Examples: .action((commentId: string, opts: { body: string }) => { const session = requireSession(); const sent = findSubmittedThreadForComment(commentId, session.id); - editComment(commentId, opts.body); + editComment(commentId, fromShell(opts.body)); if (sent) { // The forge is showing the old wording and will keep showing it; saying so is the only // honest thing available, since a posted review comment cannot be edited from here. @@ -450,7 +451,7 @@ Examples: 'new', 0, 0, - opts.body, + fromShell(opts.body), { name: 'Agent', type: 'agent' }, ); console.log(pc.green(`Created general comment ${thread.id.slice(0, 8)}`)); @@ -476,7 +477,7 @@ Examples: .option('--note ', 'What is being reviewed', '') .action((opts) => { const session = requireSession(); - startReviewRun(session.id, opts.note); + startReviewRun(session.id, fromShell(opts.note ?? '')); console.log(pc.green('Review marked as in progress')); }); @@ -537,7 +538,7 @@ Examples: .option('--json', 'Output as JSON') .action((opts) => { const session = requireSession(); - const tour = createTour(session.id, opts.topic, opts.body); + const tour = createTour(session.id, fromShell(opts.topic), fromShell(opts.body)); if (opts.json) { console.log(JSON.stringify(tour, null, 2)); return; @@ -560,7 +561,7 @@ Examples: assertFileExists(opts.file); const tourId = resolveTourId(opts.tour, session.id); const endLine = opts.endLine ?? opts.line; - const step = addTourStep(tourId, opts.file, opts.line, endLine, opts.body, opts.annotation); + const step = addTourStep(tourId, opts.file, opts.line, endLine, fromShell(opts.body), fromShell(opts.annotation)); if (opts.json) { console.log(JSON.stringify(step, null, 2)); return; diff --git a/packages/cli/src/renames.ts b/packages/cli/src/renames.ts new file mode 100644 index 0000000..56c2ad5 --- /dev/null +++ b/packages/cli/src/renames.ts @@ -0,0 +1,38 @@ +/** + * Where a file went, from `git diff -M --name-status`. + * + * Threads carry forward across a commit but their file path does not, so a commit that renames a + * file leaves every finding on it pointing at a path that no longer exists — and a thread whose + * file is absent from the diff is rendered by nothing, so it goes quiet rather than wrong. + * + * `--name-status` rather than the diff body: this runs on a poll, and the answer is a few hundred + * bytes either way. Renames only, never copies — a copy leaves the original in place, so following + * one would take a finding off the file it was written about. + */ +export function renamedPaths(nameStatus: string): Map { + const moves = new Map(); + + for (const line of nameStatus.split('\n')) { + const [status, from, to] = line.split('\t'); + if (!status?.startsWith('R') || !from || !to || from === to) continue; + moves.set(from, to); + } + + return moves; +} + +/** + * A rename can happen twice across the commits a review spans, so the chain is followed. Bounded, + * because a swap would otherwise loop. + */ +export function followRename(path: string, moves: Map): string { + const seen = new Set([path]); + let current = path; + + while (true) { + const next = moves.get(current); + if (!next || seen.has(next)) return current; + seen.add(next); + current = next; + } +} diff --git a/packages/cli/src/server.ts b/packages/cli/src/server.ts index 0610077..9b30562 100644 --- a/packages/cli/src/server.ts +++ b/packages/cli/src/server.ts @@ -382,7 +382,7 @@ export function startServer(options: ServerOptions): Promise { if (asked) { return resolveSessionId(asked); } - return findOrCreateSession(url.searchParams.get('ref') || effectiveRef).id; + return findOrCreateSession(url.searchParams.get('ref') || effectiveRef || 'work').id; }; if (pathname === '/api/live/status') { diff --git a/packages/cli/src/session.ts b/packages/cli/src/session.ts index 344fa0b..8ad4ae8 100644 --- a/packages/cli/src/session.ts +++ b/packages/cli/src/session.ts @@ -1,11 +1,12 @@ import { randomUUID } from 'node:crypto'; import { readFileSync, writeFileSync } from 'node:fs'; import { join } from 'node:path'; -import { getHeadHash, getDiffityDir, getRepoRoot, getCurrentBranch, WORKING_TREE_REFS } from '@diffity/git'; +import { getHeadHash, getDiffityDir, getRepoRoot, getCurrentBranch, getRenameStatus, WORKING_TREE_REFS } from '@diffity/git'; +import { renamedPaths, followRename } from './renames.js'; import { getDb, queryAll, queryOne } from './db.js'; import { reanchorInWorkingTree } from './anchor.js'; import { carryReviewRun } from './review-run.js'; -import { updateThreadLines } from './threads.js'; +import { updateThreadLines, updateThreadPath } from './threads.js'; export interface Session { id: string; @@ -13,6 +14,15 @@ export interface Session { headHash: string; } +/** + * `git rev-parse --abbrev-ref HEAD` says `HEAD` on a detached checkout, which is not a branch name. + * Recorded as one it matches nothing, so a session written before `gh pr checkout` put the worktree + * on a real branch is stranded along with its findings. + */ +function namedBranch(branch: string): string | null { + return branch === 'HEAD' ? null : branch; +} + function sessionFilePath(): string { return join(getDiffityDir(), 'current-session'); } @@ -61,10 +71,27 @@ function sessionsInScope( ).filter(row => branchMatches(row.branch, branch) && reviewScope(row.ref) === scope); } +/** + * Whether work may move from that session into this one. + * + * Two base refs belong to one review only because a branch's base moves as the branch is updated. + * With no branch on this side, that reasoning is gone: nothing connects one base commit to another, + * so two unrelated pull requests reviewed in one detached checkout look like a single review and the + * newer takes the older's findings. Requiring the same ref strands a review instead of merging it + * into somebody else's, which is the right way round to be wrong. + * + * Only taking work is guarded. Pointing a stale tab at a newer session shows the reader something + * they can check, and a row with no branch of its own is a session from before branches were + * recorded, which is a migration rather than a collision. + */ +function mayCarryFrom(rowRef: string, ref: string, branch: string | null): boolean { + return branch !== null || rowRef === ref; +} + export function findOrCreateSession(ref: string): Session { const headHash = getHeadHash(); const repoRoot = getRepoRoot(); - const branch = getCurrentBranch(); + const branch = namedBranch(getCurrentBranch()); const { session, created } = openSession(ref, headHash, repoRoot, branch); @@ -75,13 +102,21 @@ export function findOrCreateSession(ref: string): Session { // A superseded session is never deleted, so "a sibling exists" stays true forever and cannot be // what decides this. `/api/info` calls in here on a five-second poll, and the work below moves // rows and reads the working tree once per anchored finding. - const siblings = sessionsInScope(repoRoot, branch, ref).filter(row => row.id !== session.id); + const siblings = sessionsInScope(repoRoot, branch, ref) + .filter(row => row.id !== session.id) + .filter(row => mayCarryFrom(row.ref, ref, branch)); const donors = sessionsHoldingWork(siblings.map(row => row.id)); if (donors.length > 0) { gatherOpenWork(donors, session.id); } + // Before re-anchoring, which reads the working tree at each thread's path: a thread still + // holding a pre-rename path would find nothing there and quietly keep its old lines. + if (donors.length > 0) { + followRenamesForSession(session.id, donors, headHash); + } + if (created || donors.length > 0) { // A run belongs to the session the review was last read through — the newest sibling, which is // not necessarily one holding findings. Taking one from any older sibling would bring a review @@ -127,7 +162,7 @@ function openSession( ref: string, headHash: string, repoRoot: string | null, - branch: string, + branch: string | null, ): { session: Session; created: boolean } { const db = getDb(); @@ -221,6 +256,56 @@ function gatherOpenWork(fromSessionIds: string[], toSessionId: string): void { * A finding that outlives the commit it was written against points at a line that has since * moved. Only the new side is re-anchored: a comment on a removed line has nothing to follow. */ +function donorHeads(donorIds: string[]): string[] { + if (donorIds.length === 0) { + return []; + } + const placeholders = donorIds.map(() => '?').join(', '); + return queryAll<{ head_hash: string }>( + `SELECT DISTINCT head_hash FROM review_sessions WHERE id IN (${placeholders})`, + ...donorIds, + ).map(row => row.head_hash); +} + +/** + * A carried thread keeps the path it was written against, and a commit that renames a file leaves + * it pointing at one that is gone. Nothing renders a thread whose file is absent from the diff, so + * the finding does not look wrong — it disappears. + */ +function followRenamesForSession(sessionId: string, donorIds: string[], toHead: string): void { + const moves = new Map(); + + // Between the commit a finding was written against and the one it is carried to. The review's own + // diff is no help: it shows the file only under the name it ends up with, so the rename is not + // in it to find. + for (const from of donorHeads(donorIds)) { + if (from === toHead) continue; + try { + for (const [before, after] of renamedPaths(getRenameStatus(from, toHead))) { + moves.set(before, after); + } + } catch { + continue; + } + } + + if (moves.size === 0) { + return; + } + + const threads = queryAll<{ id: string; file_path: string }>( + "SELECT id, file_path FROM comment_threads WHERE session_id = ? AND status = 'open'", + sessionId, + ); + + for (const thread of threads) { + const moved = followRename(thread.file_path, moves); + if (moved !== thread.file_path) { + updateThreadPath(thread.id, moved); + } + } +} + function reanchorThreads(sessionId: string): void { const threads = queryAll<{ id: string; diff --git a/packages/cli/src/threads.ts b/packages/cli/src/threads.ts index 94877e0..ed2e688 100644 --- a/packages/cli/src/threads.ts +++ b/packages/cli/src/threads.ts @@ -1,6 +1,5 @@ import { randomUUID } from 'node:crypto'; import { getDb, queryAll, queryOne } from './db.js'; -import { unescapeMarkdown } from './unescape.js'; export interface ThreadAuthor { name: string; @@ -161,6 +160,12 @@ export function markThreadsSubmitted( } } +export function updateThreadPath(threadId: string, filePath: string): void { + getDb() + .prepare('UPDATE comment_threads SET file_path = ? WHERE id = ?') + .run(filePath, threadId); +} + export function updateThreadLines(threadId: string, startLine: number, endLine: number): void { const db = getDb(); db.prepare('UPDATE comment_threads SET start_line = ?, end_line = ? WHERE id = ?').run( @@ -186,7 +191,7 @@ export function createThread( const commentId = randomUUID(); const now = new Date().toISOString(); - const cleanBody = unescapeMarkdown(body); + const cleanBody = body; db.prepare( 'INSERT INTO comment_threads (id, session_id, file_path, side, start_line, end_line, anchor_content, created_at, updated_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)' @@ -304,7 +309,7 @@ export function addReply( const db = getDb(); const commentId = randomUUID(); const now = new Date().toISOString(); - const cleanBody = unescapeMarkdown(body); + const cleanBody = body; db.prepare( 'INSERT INTO comments (id, thread_id, author_name, author_type, body, kind, created_at) VALUES (?, ?, ?, ?, ?, ?, ?)' @@ -343,7 +348,7 @@ export function updateThreadStatus(threadId: string, status: ThreadStatus, summa if (summaryBody && summaryAuthor) { const commentId = randomUUID(); - const cleanSummary = unescapeMarkdown(summaryBody); + const cleanSummary = summaryBody; db.prepare( 'INSERT INTO comments (id, thread_id, author_name, author_type, body, created_at) VALUES (?, ?, ?, ?, ?, ?)' ).run(commentId, threadId, summaryAuthor.name, summaryAuthor.type, cleanSummary, now); @@ -362,7 +367,7 @@ export function deleteAllThreadsForSession(sessionId: string): void { export function editComment(commentId: string, body: string): void { const db = getDb(); - db.prepare('UPDATE comments SET body = ? WHERE id = ?').run(unescapeMarkdown(body), commentId); + db.prepare('UPDATE comments SET body = ? WHERE id = ?').run(body, commentId); } export function deleteComment(commentId: string): void { diff --git a/packages/cli/src/tours.ts b/packages/cli/src/tours.ts index a696bb6..108dab1 100644 --- a/packages/cli/src/tours.ts +++ b/packages/cli/src/tours.ts @@ -1,6 +1,5 @@ import { randomUUID } from 'node:crypto'; import { getDb, queryAll, queryOne } from './db.js'; -import { unescapeMarkdown } from './unescape.js'; export type TourStatus = 'building' | 'ready'; @@ -78,7 +77,7 @@ export function createTour(sessionId: string, topic: string, body: string): Tour const id = randomUUID(); const now = new Date().toISOString(); - const cleanBody = unescapeMarkdown(body); + const cleanBody = body; db.prepare( 'INSERT INTO tours (id, session_id, topic, body, created_at) VALUES (?, ?, ?, ?, ?)' @@ -180,8 +179,8 @@ export function addTourStep( ); const sortOrder = (maxRow?.max_order ?? 0) + 1; - const cleanBody = unescapeMarkdown(body); - const cleanAnnotation = unescapeMarkdown(annotation); + const cleanBody = body; + const cleanAnnotation = annotation; db.prepare( 'INSERT INTO tour_steps (id, tour_id, sort_order, file_path, start_line, end_line, body, annotation, created_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)' diff --git a/packages/cli/src/unescape.ts b/packages/cli/src/unescape.ts index b422595..293e7a4 100644 --- a/packages/cli/src/unescape.ts +++ b/packages/cli/src/unescape.ts @@ -1,13 +1,12 @@ /** - * Remove shell-introduced backslash escapes from markdown text. + * Undo the escaping a shell string picks up on its way in. * - * AI agents calling the CLI via shell commands often produce body text - * with escaped backticks (\`) and escaped quotes (\") because they - * are constructing shell strings. These backslashes become markdown - * escape sequences that suppress formatting (e.g. \` renders as a - * literal backtick instead of inline code). + * An agent building a `--body "..."` argument escapes backticks and quotes, and those backslashes + * survive into the text, where markdown reads them as escapes and stops formatting. * - * This function reverses those escapes so the markdown renders correctly. + * Only ever apply this to text that came through a shell. It is lossy in the other direction: a + * comment discussing `split('\n')` has that turned into a real line break, so text typed into the + * page must reach the database exactly as written. */ export function unescapeMarkdown(text: string): string { return text diff --git a/packages/cli/tests/renames.test.ts b/packages/cli/tests/renames.test.ts new file mode 100644 index 0000000..8807892 --- /dev/null +++ b/packages/cli/tests/renames.test.ts @@ -0,0 +1,50 @@ +import { describe, it, expect } from 'vitest'; +import { renamedPaths, followRename } from '../src/renames.js'; + +describe('renamedPaths', () => { + it('reads a rename out of --name-status', () => { + const moves = renamedPaths('R100\tscripts/medical/const.ts\tscripts/llm/const.ts'); + + expect(moves.get('scripts/medical/const.ts')).toBe('scripts/llm/const.ts'); + }); + + it('takes a partial rename too, which is any R with a similarity below 100', () => { + expect(renamedPaths('R087\told.ts\tnew.ts').get('old.ts')).toBe('new.ts'); + }); + + it('reads several', () => { + expect(renamedPaths('R100\ta.ts\tb.ts\nR100\tc.ts\td.ts').size).toBe(2); + }); + + // A copy leaves the original in place, so following it would take a finding off the file it was + // written about. git only reports copies under -C, which is not asked for. + it('is not fooled by a copy', () => { + expect(renamedPaths('C100\ta.ts\tb.ts').size).toBe(0); + }); + + it('ignores everything that is not a rename', () => { + expect(renamedPaths('M\ta.ts\nA\tb.ts\nD\tc.ts\n').size).toBe(0); + }); + + it('is empty on empty output', () => { + expect(renamedPaths('').size).toBe(0); + expect(renamedPaths('\n\n').size).toBe(0); + }); +}); + +describe('followRename', () => { + const moves = new Map([['a.ts', 'b.ts'], ['b.ts', 'c.ts']]); + + it('leaves a path nothing moved', () => { + expect(followRename('untouched.ts', moves)).toBe('untouched.ts'); + }); + + // A review can span several commits, and a file can be moved twice across them. + it('follows a chain', () => { + expect(followRename('a.ts', moves)).toBe('c.ts'); + }); + + it('stops rather than looping when two files swap', () => { + expect(followRename('x.ts', new Map([['x.ts', 'y.ts'], ['y.ts', 'x.ts']]))).toBe('y.ts'); + }); +}); diff --git a/packages/cli/tests/session-detached.test.ts b/packages/cli/tests/session-detached.test.ts new file mode 100644 index 0000000..21404ea --- /dev/null +++ b/packages/cli/tests/session-detached.test.ts @@ -0,0 +1,116 @@ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import { mkdtempSync, writeFileSync, rmSync } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; + +let root: string; +let repoDir: string; +let origCwd: string; + +function git(args: string[]): string { + return execFileSync('git', args, { cwd: repoDir, encoding: 'utf-8', stdio: 'pipe' }).trim(); +} + +function commit(name: string, body: string): string { + writeFileSync(join(repoDir, name), body); + git(['add', '.']); + git(['commit', '-m', name]); + return git(['rev-parse', 'HEAD']); +} + +const agent = { name: 'Agent', type: 'agent' as const }; + +beforeAll(() => { + origCwd = process.cwd(); + root = mkdtempSync(join(tmpdir(), 'diffity-detached-')); + repoDir = join(root, 'repo'); + execFileSync('git', ['init', '-b', 'main', repoDir], { stdio: 'pipe' }); + git(['config', 'user.email', 't@t']); + git(['config', 'user.name', 'T']); + commit('a.txt', 'a\n'); + process.env.DIFFITY_DATA_DIR = join(root, 'notes'); + process.chdir(repoDir); +}); + +afterAll(() => { + process.chdir(origCwd); + delete process.env.DIFFITY_DATA_DIR; + rmSync(root, { recursive: true, force: true }); +}); + +describe('a detached checkout, which is how a reviewer opens somebody else pull request', () => { + it('does not record HEAD as though it were a branch', async () => { + const { findOrCreateSession } = await import('../src/session.js'); + const { getDb } = await import('../src/db.js'); + + const head = git(['rev-parse', 'HEAD']); + git(['checkout', '--detach', head]); + const session = findOrCreateSession('detached-base'); + const row = getDb() + .prepare('SELECT branch FROM review_sessions WHERE id = ?') + .get(session.id) as { branch: string | null }; + + expect(row.branch).toBeNull(); + git(['checkout', 'main']); + }); + + // The sequence that stranded findings: diffity runs on a detached worktree, then PR mode calls + // `gh pr checkout` and the next run is on a real branch. Recorded as a branch called HEAD, the + // first session matched neither the detached run nor the one after it. + it('keeps its findings once the checkout lands on a real branch', async () => { + const { findOrCreateSession } = await import('../src/session.js'); + const { createThread, getThreadsForSession } = await import('../src/threads.js'); + + git(['checkout', '--detach', git(['rev-parse', 'HEAD'])]); + const whileDetached = findOrCreateSession('handover-base'); + const finding = createThread(whileDetached.id, 'a.txt', 'new', 1, 1, 'found while detached', agent); + + git(['checkout', '-b', 'pr-branch']); + const onBranch = findOrCreateSession('handover-base'); + + // Same commit and same base, so the branchless row is adopted in place rather than superseded. + expect(onBranch.id).toBe(whileDetached.id); + expect(getThreadsForSession(onBranch.id).map(t => t.id)).toContain(finding.id); + git(['checkout', 'main']); + }); + + it('carries them forward when the commit moves under it as well', async () => { + const { findOrCreateSession } = await import('../src/session.js'); + const { createThread, getThreadsForSession } = await import('../src/threads.js'); + + git(['checkout', '--detach', git(['rev-parse', 'HEAD'])]); + const before = findOrCreateSession('moving-base'); + const finding = createThread(before.id, 'a.txt', 'new', 1, 1, 'found before the commit moved', agent); + + git(['checkout', '-B', 'moved-branch']); + commit('d.txt', 'd\n'); + const after = findOrCreateSession('moving-base'); + + expect(after.id).not.toBe(before.id); + expect(getThreadsForSession(after.id).map(t => t.id)).toContain(finding.id); + git(['checkout', 'main']); + }); + + // What swallowed a whole review: with no branch, every base ref shares one scope, so a session + // opened for a second pull request looked like the same review continuing. + it('does not take the findings of a different review in the same checkout', async () => { + const { findOrCreateSession } = await import('../src/session.js'); + const { createThread, getThreadsForSession } = await import('../src/threads.js'); + + const firstBase = commit('b.txt', 'b\n'); + git(['checkout', '--detach', firstBase]); + const reviewOne = findOrCreateSession(firstBase); + const finding = createThread(reviewOne.id, 'a.txt', 'new', 1, 1, 'belongs to review one', agent); + + git(['checkout', 'main']); + const secondBase = commit('c.txt', 'c\n'); + git(['checkout', '--detach', secondBase]); + const reviewTwo = findOrCreateSession(secondBase); + + expect(reviewTwo.id).not.toBe(reviewOne.id); + expect(getThreadsForSession(reviewTwo.id).map(t => t.id)).not.toContain(finding.id); + expect(getThreadsForSession(reviewOne.id).map(t => t.id)).toContain(finding.id); + git(['checkout', 'main']); + }); +}); diff --git a/packages/cli/tests/session-rename.test.ts b/packages/cli/tests/session-rename.test.ts new file mode 100644 index 0000000..2dfb9c5 --- /dev/null +++ b/packages/cli/tests/session-rename.test.ts @@ -0,0 +1,81 @@ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import { mkdtempSync, writeFileSync, rmSync } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; + +let root: string; +let repoDir: string; +let origCwd: string; + +function git(args: string[]): string { + return execFileSync('git', args, { cwd: repoDir, encoding: 'utf-8', stdio: 'pipe' }).trim(); +} + +const agent = { name: 'Agent', type: 'agent' as const }; +const BODY = 'export const LIMIT = 5;\n'; + +beforeAll(() => { + origCwd = process.cwd(); + root = mkdtempSync(join(tmpdir(), 'diffity-rename-')); + repoDir = join(root, 'repo'); + execFileSync('git', ['init', '-b', 'main', repoDir], { stdio: 'pipe' }); + git(['config', 'user.email', 't@t']); + git(['config', 'user.name', 'T']); + writeFileSync(join(repoDir, 'base.txt'), 'base\n'); + git(['add', '.']); + git(['commit', '-m', 'init']); + git(['branch', 'base']); + process.env.DIFFITY_DATA_DIR = join(root, 'notes'); + process.chdir(repoDir); +}); + +afterAll(() => { + process.chdir(origCwd); + delete process.env.DIFFITY_DATA_DIR; + rmSync(root, { recursive: true, force: true }); +}); + +// What bit: a finding written against `scripts/medical/const.ts`, then a commit that renames the +// file. The thread carries forward and keeps the old path, which nothing in the page renders. +describe('a finding on a file that a later commit renames', () => { + it('moves to the path the file now has', async () => { + const { findOrCreateSession } = await import('../src/session.js'); + const { createThread, getThreadsForSession } = await import('../src/threads.js'); + + writeFileSync(join(repoDir, 'old-name.ts'), BODY); + git(['add', '.']); + git(['commit', '-m', 'add the file']); + + const before = findOrCreateSession('base'); + const finding = createThread(before.id, 'old-name.ts', 'new', 1, 1, 'P2: magic number', agent, BODY.trim()); + expect(getThreadsForSession(before.id)[0].filePath).toBe('old-name.ts'); + + git(['mv', 'old-name.ts', 'new-name.ts']); + git(['commit', '-m', 'rename it']); + + const after = findOrCreateSession('base'); + + expect(after.id).not.toBe(before.id); + const carried = getThreadsForSession(after.id).find(t => t.id === finding.id); + expect(carried).toBeDefined(); + expect(carried!.filePath).toBe('new-name.ts'); + }); + + it('leaves a finding on a file nothing renamed where it is', async () => { + const { findOrCreateSession } = await import('../src/session.js'); + const { createThread, getThreadsForSession } = await import('../src/threads.js'); + + const before = findOrCreateSession('base'); + const finding = createThread(before.id, 'base.txt', 'new', 1, 1, 'P3: a note', agent, 'base'); + + writeFileSync(join(repoDir, 'another.ts'), 'x\n'); + git(['add', '.']); + git(['commit', '-m', 'unrelated']); + + const after = findOrCreateSession('base'); + const carried = getThreadsForSession(after.id).find(t => t.id === finding.id); + + expect(carried!.filePath).toBe('base.txt'); + }); +}); diff --git a/packages/cli/tests/verbatim-storage.test.ts b/packages/cli/tests/verbatim-storage.test.ts new file mode 100644 index 0000000..de276c7 --- /dev/null +++ b/packages/cli/tests/verbatim-storage.test.ts @@ -0,0 +1,74 @@ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import { mkdtempSync, writeFileSync, rmSync } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; + +let root: string; +let repoDir: string; +let origCwd: string; + +beforeAll(() => { + origCwd = process.cwd(); + root = mkdtempSync(join(tmpdir(), 'diffity-verbatim-')); + repoDir = join(root, 'repo'); + execFileSync('git', ['init', '-b', 'main', repoDir], { stdio: 'pipe' }); + execFileSync('git', ['config', 'user.email', 't@t'], { cwd: repoDir, stdio: 'pipe' }); + execFileSync('git', ['config', 'user.name', 'T'], { cwd: repoDir, stdio: 'pipe' }); + writeFileSync(join(repoDir, 'a.ts'), 'const a = 1;\n'); + execFileSync('git', ['add', '.'], { cwd: repoDir, stdio: 'pipe' }); + execFileSync('git', ['commit', '-m', 'init'], { cwd: repoDir, stdio: 'pipe' }); + process.env.DIFFITY_DATA_DIR = join(root, 'notes'); + process.chdir(repoDir); +}); + +afterAll(() => { + process.chdir(origCwd); + delete process.env.DIFFITY_DATA_DIR; + rmSync(root, { recursive: true, force: true }); +}); + +const user = { name: 'You', type: 'user' as const }; + +// The page has no shell in it, so nothing typed there needs unescaping — and unescaping is lossy. +// A comment about `split('\n')` used to reach the database with a real line break in it. +const AS_TYPED = "why not split('\\n') here? the \\\" is deliberate, and \\` too"; + +describe('text that never went through a shell', () => { + it('is stored exactly as written', async () => { + const { findOrCreateSession } = await import('../src/session.js'); + const { createThread, getThread } = await import('../src/threads.js'); + + const session = findOrCreateSession('work'); + const thread = createThread(session.id, 'a.ts', 'new', 1, 1, AS_TYPED, user); + + expect(getThread(thread.id)!.comments[0].body).toBe(AS_TYPED); + }); + + it('survives a reply and an edit too', async () => { + const { findOrCreateSession } = await import('../src/session.js'); + const { createThread, addReply, editComment, getThread } = await import('../src/threads.js'); + + const session = findOrCreateSession('work'); + const thread = createThread(session.id, 'a.ts', 'new', 1, 1, 'the finding', user); + const reply = addReply(thread.id, AS_TYPED, user); + expect(getThread(thread.id)!.comments[1].body).toBe(AS_TYPED); + + editComment(reply.id, AS_TYPED); + expect(getThread(thread.id)!.comments[1].body).toBe(AS_TYPED); + }); + + it('keeps a walkthrough step as written', async () => { + const { findOrCreateSession } = await import('../src/session.js'); + const { createTour, addTourStep, getTour } = await import('../src/tours.js'); + + const session = findOrCreateSession('work'); + const tour = createTour(session.id, 'Reading order', AS_TYPED); + addTourStep(tour.id, 'a.ts', 1, 1, AS_TYPED, AS_TYPED); + + const stored = getTour(tour.id)!; + expect(stored.body).toBe(AS_TYPED); + expect(stored.steps[0].body).toBe(AS_TYPED); + expect(stored.steps[0].annotation).toBe(AS_TYPED); + }); +}); diff --git a/packages/git/package.json b/packages/git/package.json index 31cc63d..dc4d82c 100644 --- a/packages/git/package.json +++ b/packages/git/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/git", - "version": "0.9.10", + "version": "0.9.13", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/git/src/diff.ts b/packages/git/src/diff.ts index b2128b8..20449a3 100644 --- a/packages/git/src/diff.ts +++ b/packages/git/src/diff.ts @@ -73,6 +73,14 @@ export function resolveRef(ref: string, extraArgs: string[] = []): string { return raw; } +/** + * Which files a commit range renamed, as `R100\told\tnew` lines. `-M` explicitly rather than + * relying on the default, which `diff.renames = false` turns off. + */ +export function getRenameStatus(from: string, to: string): string { + return git(['diff', '-M', '--name-status', '--diff-filter=R', from, to]); +} + export function getDiffFiles(ref: string): string[] { const resolved = resolveDiffArgs(ref); diff --git a/packages/git/src/index.ts b/packages/git/src/index.ts index 52ba783..e1a1783 100644 --- a/packages/git/src/index.ts +++ b/packages/git/src/index.ts @@ -1,7 +1,7 @@ export type { Commit, RepoInfo } from './types.js'; export type { RefCapabilities } from './repo.js'; export { isGitRepo, getRepoRoot, getRepoName, getCurrentBranch, getRepoInfo, getHeadHash, getDiffityDir, getDiffityDirPath, isDataDirUntracked, getRefCapabilities, isValidGitRef } from './repo.js'; -export { getDiff, getDiffFiles, getDiffStat, getDiffStatForRef, getUntrackedFiles, getUntrackedDiff, getFileContent, getFileLineCount, getMergeBase, normalizeRef, resolveBaseRef, resolveThroughUpstream, resolveDiffArgs, resolveRef, revertFile, revertHunk, WORKING_TREE_REFS } from './diff.js'; +export { getDiff, getDiffFiles, getDiffStat, getDiffStatForRef, getRenameStatus, getUntrackedFiles, getUntrackedDiff, getFileContent, getFileLineCount, getMergeBase, normalizeRef, resolveBaseRef, resolveThroughUpstream, resolveDiffArgs, resolveRef, revertFile, revertHunk, WORKING_TREE_REFS } from './diff.js'; export type { RefDiffArgs } from './diff.js'; export { getStagedFiles, getUnstagedFiles, isDirty } from './status.js'; export { getRecentCommits } from './commits.js'; diff --git a/packages/github/package.json b/packages/github/package.json index c680bf7..439a461 100644 --- a/packages/github/package.json +++ b/packages/github/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/github", - "version": "0.9.10", + "version": "0.9.13", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/github/src/pr.ts b/packages/github/src/pr.ts index ad7f069..9cf2d0f 100644 --- a/packages/github/src/pr.ts +++ b/packages/github/src/pr.ts @@ -235,7 +235,7 @@ export function createReview( // The whole review is one request, so a single unpostable line would reject all of it. if (!sides[comment.side].has(comment.endLine)) { errors.push( - `${comment.filePath}:${comment.endLine} — outside the lines this PR changed, so the forge will not take a comment there`, + `${comment.filePath}:${comment.endLine} — not in the diff diffity fetched for this pull request, so the forge will not take a comment there. If the line is really in the diff, the local branch and the pull request have drifted`, ); continue; } @@ -289,9 +289,17 @@ export function createReview( } } +/** + * The pull request's diff as the review API sees it: one section per file, base to head. + * + * Not `--patch`, which is the commit series in mbox form — one section per commit, hunks measured + * against that commit's parent, and files under whatever path they had at the time. A file touched + * by several commits appears several times, and only the last one survives being collected, so + * whether a comment was allowed depended on which commit happened to touch that line last. + */ function getPatch(owner: string, repo: string, prNumber: number): string { try { - return execFileSync('gh', ['pr', 'diff', String(prNumber), '--repo', `${owner}/${repo}`, '--patch'], { + return execFileSync('gh', ['pr', 'diff', String(prNumber), '--repo', `${owner}/${repo}`], { encoding: 'utf-8', stdio: 'pipe', maxBuffer: 50 * 1024 * 1024, diff --git a/packages/parser/package.json b/packages/parser/package.json index 27898f5..77d00aa 100644 --- a/packages/parser/package.json +++ b/packages/parser/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/parser", - "version": "0.9.10", + "version": "0.9.13", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/ui/package.json b/packages/ui/package.json index 0858311..1ac3bf2 100644 --- a/packages/ui/package.json +++ b/packages/ui/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/ui", - "version": "0.9.10", + "version": "0.9.13", "type": "module", "private": true, "scripts": { diff --git a/packages/ui/src/components/diff/diff-view.tsx b/packages/ui/src/components/diff/diff-view.tsx index fff7784..aa4a446 100644 --- a/packages/ui/src/components/diff/diff-view.tsx +++ b/packages/ui/src/components/diff/diff-view.tsx @@ -2,6 +2,8 @@ import { useMemo, useRef, useState, useCallback, useImperativeHandle, useEffect import { useVirtualizer } from '@tanstack/react-virtual'; import type { ParsedDiff } from '@diffity/parser'; import { FileBlock, LARGE_DIFF_LINE_THRESHOLD } from './file-block'; +import { OrphanedThreads } from '../comments/orphaned-threads'; +import { threadsWithoutFile } from '../../lib/threads-without-file'; import { GeneralComments } from '../comments/general-comments'; import { useHighlighter } from '../../hooks/use-highlighter'; import { type ViewMode, getFilePath } from '../../lib/diff-utils'; @@ -318,6 +320,11 @@ export function DiffView(props: DiffViewProps) { ] : [0, 0]; + const lostThreads = useMemo( + () => threadsWithoutFile(threads, diff.files.map(getFilePath)), + [threads, diff.files], + ); + return (
{ @@ -335,6 +342,16 @@ export function DiffView(props: DiffViewProps) { commentActions={commentActions} /> )} + {commentsEnabled && lostThreads.length > 0 && ( +
+ +
+ )}
{items.map((virtualItem) => { const file = diff.files[virtualItem.index]; diff --git a/packages/ui/src/components/layout/dashboard.tsx b/packages/ui/src/components/layout/dashboard.tsx index 619e7f1..6fc1975 100644 --- a/packages/ui/src/components/layout/dashboard.tsx +++ b/packages/ui/src/components/layout/dashboard.tsx @@ -14,9 +14,11 @@ export function Dashboard(props: DashboardProps) { const { onNavigate } = props; const { data: overview, loading: overviewLoading, error } = useOverview(); const { data: commitsPage, loading: commitsLoading } = useCommits(); - const { data: info, loading: infoLoading } = useInfo(); + // `useInfo` suspends, so its data is there by the time this renders and there is no + // loading to ask about. Asking anyway read as undefined and contributed nothing. + const { data: info } = useInfo(); - const anyLoading = overviewLoading || commitsLoading || infoLoading; + const anyLoading = overviewLoading || commitsLoading; if (error) { return ( diff --git a/packages/ui/src/lib/threads-without-file.ts b/packages/ui/src/lib/threads-without-file.ts new file mode 100644 index 0000000..1f170a9 --- /dev/null +++ b/packages/ui/src/lib/threads-without-file.ts @@ -0,0 +1,24 @@ +import type { CommentThread } from '../components/comments/types'; +import { GENERAL_THREAD_FILE_PATH, isThreadResolved } from '../components/comments/types'; + +/** + * Findings whose file is not in the diff at all. + * + * A thread is rendered inside the block for its file, so one naming a file that is not there is + * rendered by nothing — it does not look wrong, it is simply absent, which is the worst way for a + * finding to be lost. Renames are followed when work carries forward, so what reaches here is a + * file that was deleted, or moved in a way git did not report as a rename. + * + * Resolved threads are left out: they have been dealt with, and the file going away is no reason to + * ask about them again. + */ +export function threadsWithoutFile(threads: CommentThread[], diffPaths: Iterable): CommentThread[] { + const present = new Set(diffPaths); + + return threads.filter( + thread => + thread.filePath !== GENERAL_THREAD_FILE_PATH + && !isThreadResolved(thread) + && !present.has(thread.filePath), + ); +} diff --git a/packages/ui/tests/threads-without-file.test.ts b/packages/ui/tests/threads-without-file.test.ts new file mode 100644 index 0000000..94cbc11 --- /dev/null +++ b/packages/ui/tests/threads-without-file.test.ts @@ -0,0 +1,49 @@ +import { describe, it, expect } from 'vitest'; +import { threadsWithoutFile } from '../src/lib/threads-without-file'; +import { GENERAL_THREAD_FILE_PATH } from '../src/components/comments/types'; +import type { CommentThread } from '../src/components/comments/types'; + +function thread(over: Partial = {}): CommentThread { + return { + id: 't1', + filePath: 'src/a.ts', + side: 'new', + startLine: 1, + endLine: 1, + status: 'open', + comments: [], + ...over, + } as CommentThread; +} + +const inDiff = ['src/a.ts', 'src/b.ts']; + +describe('threadsWithoutFile', () => { + it('finds one whose file is gone', () => { + const lost = thread({ id: 'lost', filePath: 'src/renamed-away.ts' }); + + expect(threadsWithoutFile([thread(), lost], inDiff).map(t => t.id)).toEqual(['lost']); + }); + + it('leaves the ones whose file is there', () => { + expect(threadsWithoutFile([thread(), thread({ filePath: 'src/b.ts' })], inDiff)).toEqual([]); + }); + + it('is not about general comments, which have no file', () => { + expect(threadsWithoutFile([thread({ filePath: GENERAL_THREAD_FILE_PATH })], inDiff)).toEqual([]); + }); + + // Already dealt with. The file going away is not a reason to ask again. + it('ignores resolved and dismissed ones', () => { + const gone = { filePath: 'src/gone.ts' }; + + expect(threadsWithoutFile( + [thread({ ...gone, status: 'resolved' }), thread({ ...gone, status: 'dismissed' })], + inDiff, + )).toEqual([]); + }); + + it('reports everything when the diff is empty', () => { + expect(threadsWithoutFile([thread()], []).map(t => t.filePath)).toEqual(['src/a.ts']); + }); +}); diff --git a/packages/ui/tsconfig.json b/packages/ui/tsconfig.json index 0c32e76..a5994bf 100644 --- a/packages/ui/tsconfig.json +++ b/packages/ui/tsconfig.json @@ -2,7 +2,6 @@ "extends": "../../tsconfig.json", "compilerOptions": { "outDir": "./dist", - "rootDir": "./src", "rootDirs": [".", "./.react-router/types"], "composite": true, "jsx": "react-jsx",