From a43c092b82d0e1bd5d6005a0fca57034b5e80b71 Mon Sep 17 00:00:00 2001 From: "Fredrik Liljegren (Claude Code Claude Opus 5)" Date: Wed, 26 Aug 2026 09:55:57 +0200 Subject: [PATCH 1/3] fix: the six known bugs, and a typecheck that will notice the next one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **A review taken by an unrelated one.** With no branch to go on, every base ref shares a scope, so a second pull request opened in the same detached checkout looked like the same review continuing and took the first one's findings. Work may now only move between sessions when a branch connects them, or when the ref is identical. Redirecting a stale tab is left alone: showing somebody a newer session is recoverable, moving their findings is not. **A detached checkout called its branch HEAD.** `rev-parse --abbrev-ref` says `HEAD` when detached, which is not a branch name, and recorded as one it matched nothing — so a session opened before `gh pr checkout` put the worktree on a real branch was stranded with its findings. Unknown is what it is. **The wrong diff decided where a comment could go.** `gh pr diff --patch` is the commit series in mbox form, not the pull request's diff: one section per commit, hunks against that commit's parent, files under their historical paths. A file touched twice appears twice and only the last survives being collected. On our own #34 that left 25 of 89 commentable lines in one file, and the review API rejected the rest. The message blamed the reader's line arithmetic; it now names the diff. **Text typed into the page was rewritten.** Shell unescaping sat in the storage layer, which the browser routes share, so a comment about `split('\n')` reached the database with a real line break. It belongs at the CLI boundary, where a shell string actually arrives. **And the two type errors** nothing was running: a `string | undefined` passed as a ref, and a `loading` read off a suspense hook that has none. The reason both survived is that no typecheck ran. `packages/cli` had no typecheck script at all, and the UI's failed on four TS6059 errors from its own tsconfig, so its output had been noise for long enough to be ignored. Both are fixed, `npm test` runs them, and the count is zero. v0.9.11. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018PkYQzbsnMihHesafWvXKs --- package-lock.json | 10 +- package.json | 5 +- packages/cli/package.json | 5 +- packages/cli/src/agent.ts | 19 +-- packages/cli/src/server.ts | 2 +- packages/cli/src/session.ts | 34 ++++- packages/cli/src/threads.ts | 9 +- packages/cli/src/tours.ts | 7 +- packages/cli/src/unescape.ts | 13 +- packages/cli/tests/session-detached.test.ts | 116 ++++++++++++++++++ packages/cli/tests/verbatim-storage.test.ts | 74 +++++++++++ packages/git/package.json | 2 +- packages/github/package.json | 2 +- packages/github/src/pr.ts | 12 +- packages/parser/package.json | 2 +- packages/ui/package.json | 2 +- .../ui/src/components/layout/dashboard.tsx | 6 +- packages/ui/tsconfig.json | 1 - 18 files changed, 274 insertions(+), 47 deletions(-) create mode 100644 packages/cli/tests/session-detached.test.ts create mode 100644 packages/cli/tests/verbatim-storage.test.ts diff --git a/package-lock.json b/package-lock.json index 24a3e9d..169d823 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8452,7 +8452,7 @@ }, "packages/cli": { "name": "diffity", - "version": "0.9.10", + "version": "0.9.11", "license": "MIT", "dependencies": { "commander": "^14.0.3", @@ -8476,7 +8476,7 @@ }, "packages/git": { "name": "@diffity/git", - "version": "0.9.10", + "version": "0.9.11", "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.11", "dependencies": { "@diffity/parser": "*" }, @@ -8497,7 +8497,7 @@ }, "packages/parser": { "name": "@diffity/parser", - "version": "0.9.10", + "version": "0.9.11", "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.11", "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..8e39877 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -1,6 +1,6 @@ { "name": "diffity", - "version": "0.9.10", + "version": "0.9.11", "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/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..82bbf74 100644 --- a/packages/cli/src/session.ts +++ b/packages/cli/src/session.ts @@ -13,6 +13,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 +70,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,7 +101,9 @@ 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) { @@ -127,7 +155,7 @@ function openSession( ref: string, headHash: string, repoRoot: string | null, - branch: string, + branch: string | null, ): { session: Session; created: boolean } { const db = getDb(); diff --git a/packages/cli/src/threads.ts b/packages/cli/src/threads.ts index 94877e0..ae5fdeb 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; @@ -186,7 +185,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 +303,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 +342,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 +361,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/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/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..b2ee0a6 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.11", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/github/package.json b/packages/github/package.json index c680bf7..13cfeaf 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.11", "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..3ced611 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.11", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/ui/package.json b/packages/ui/package.json index 0858311..0943678 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.11", "type": "module", "private": true, "scripts": { 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/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", From 01069e78e72077beb3cfe50b14aefdfd27dc7f40 Mon Sep 17 00:00:00 2001 From: "Fredrik Liljegren (Claude Code Claude Opus 5)" Date: Wed, 26 Aug 2026 10:12:34 +0200 Subject: [PATCH 2/3] fix: a finding survives its file being renamed, and says so when it cannot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A thread carries forward across a commit; its file path did not. So a commit that renamed a file left every open finding on it naming a path that no longer exists — and since a thread is rendered inside the block for its file, one naming a file that is not there is rendered by nothing. The finding did not look wrong. It was gone. Renames are followed from git's own detection, between the commit the finding was written against and the one it is carried to. Not the review's own diff, which shows the file only under the name it ends up with, so the rename is not in it to find. Content matching was the other option and this is better: a thread only moves to a file git already called the same file. That leaves deletions, and moves git does not report. Those now appear at the top of the diff instead of nowhere, through the same component that already handles a thread whose line has gone. v0.9.12. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018PkYQzbsnMihHesafWvXKs --- package-lock.json | 10 +-- packages/cli/package.json | 2 +- packages/cli/src/renames.ts | 43 ++++++++++ packages/cli/src/session.ts | 61 +++++++++++++- packages/cli/src/threads.ts | 6 ++ packages/cli/tests/renames.test.ts | 48 +++++++++++ packages/cli/tests/session-rename.test.ts | 81 +++++++++++++++++++ packages/git/package.json | 2 +- packages/github/package.json | 2 +- packages/parser/package.json | 2 +- packages/ui/package.json | 2 +- packages/ui/src/components/diff/diff-view.tsx | 17 ++++ packages/ui/src/lib/threads-without-file.ts | 24 ++++++ .../ui/tests/threads-without-file.test.ts | 49 +++++++++++ 14 files changed, 337 insertions(+), 12 deletions(-) create mode 100644 packages/cli/src/renames.ts create mode 100644 packages/cli/tests/renames.test.ts create mode 100644 packages/cli/tests/session-rename.test.ts create mode 100644 packages/ui/src/lib/threads-without-file.ts create mode 100644 packages/ui/tests/threads-without-file.test.ts diff --git a/package-lock.json b/package-lock.json index 169d823..d857314 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8452,7 +8452,7 @@ }, "packages/cli": { "name": "diffity", - "version": "0.9.11", + "version": "0.9.12", "license": "MIT", "dependencies": { "commander": "^14.0.3", @@ -8476,7 +8476,7 @@ }, "packages/git": { "name": "@diffity/git", - "version": "0.9.11", + "version": "0.9.12", "devDependencies": { "@types/node": "^25.5.0", "typescript": "^5.9.3", @@ -8485,7 +8485,7 @@ }, "packages/github": { "name": "@diffity/github", - "version": "0.9.11", + "version": "0.9.12", "dependencies": { "@diffity/parser": "*" }, @@ -8497,7 +8497,7 @@ }, "packages/parser": { "name": "@diffity/parser", - "version": "0.9.11", + "version": "0.9.12", "devDependencies": { "typescript": "^5.9.3", "vitest": "^4.1.0" @@ -8505,7 +8505,7 @@ }, "packages/ui": { "name": "@diffity/ui", - "version": "0.9.11", + "version": "0.9.12", "dependencies": { "@react-router/node": "^7.13.2", "@tailwindcss/vite": "^4.2.1", diff --git a/packages/cli/package.json b/packages/cli/package.json index 8e39877..de52228 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -1,6 +1,6 @@ { "name": "diffity", - "version": "0.9.11", + "version": "0.9.12", "description": "GitHub-style git diff viewer in the browser", "type": "module", "bin": { diff --git a/packages/cli/src/renames.ts b/packages/cli/src/renames.ts new file mode 100644 index 0000000..3a0ee7c --- /dev/null +++ b/packages/cli/src/renames.ts @@ -0,0 +1,43 @@ +interface FileLike { + status: string; + oldPath: string; + newPath: string; +} + +/** + * Where a file went, for the files git says moved. + * + * 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. + * + * Taken from git's own rename detection rather than by matching content, so a thread only ever + * moves to a file git already said is the same file. + */ +export function renamedPaths(files: FileLike[]): Map { + const moves = new Map(); + + for (const file of files) { + if (file.status !== 'renamed' && file.status !== 'copied') continue; + if (!file.oldPath || !file.newPath || file.oldPath === file.newPath) continue; + moves.set(file.oldPath, file.newPath); + } + + 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/session.ts b/packages/cli/src/session.ts index 82bbf74..072b08e 100644 --- a/packages/cli/src/session.ts +++ b/packages/cli/src/session.ts @@ -1,11 +1,13 @@ 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, getDiff, WORKING_TREE_REFS } from '@diffity/git'; +import { parseDiff } from '@diffity/parser'; +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; @@ -110,6 +112,10 @@ export function findOrCreateSession(ref: string): Session { gatherOpenWork(donors, session.id); } + 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 @@ -249,6 +255,57 @@ 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 { + // `-M` explicitly rather than relying on the default, which `diff.renames = false` turns off. + for (const [before, after] of renamedPaths(parseDiff(getDiff(['-M', from, toHead])).files)) { + 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 ae5fdeb..ed2e688 100644 --- a/packages/cli/src/threads.ts +++ b/packages/cli/src/threads.ts @@ -160,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( diff --git a/packages/cli/tests/renames.test.ts b/packages/cli/tests/renames.test.ts new file mode 100644 index 0000000..dac588a --- /dev/null +++ b/packages/cli/tests/renames.test.ts @@ -0,0 +1,48 @@ +import { describe, it, expect } from 'vitest'; +import { renamedPaths, followRename } from '../src/renames.js'; + +const file = (status: string, oldPath: string, newPath: string) => ({ status, oldPath, newPath }); + +describe('renamedPaths', () => { + it('takes what git called a rename', () => { + const moves = renamedPaths([file('renamed', 'scripts/medical/const.ts', 'scripts/llm/const.ts')]); + + expect(moves.get('scripts/medical/const.ts')).toBe('scripts/llm/const.ts'); + }); + + it('takes a copy too, since the old path may be gone either way', () => { + expect(renamedPaths([file('copied', 'a.ts', 'b.ts')]).get('a.ts')).toBe('b.ts'); + }); + + it('ignores files that did not move', () => { + const moves = renamedPaths([ + file('modified', 'a.ts', 'a.ts'), + file('added', '', 'b.ts'), + file('deleted', 'c.ts', ''), + file('renamed', 'd.ts', 'd.ts'), + ]); + + expect(moves.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'); + }); + + it('follows a rename', () => { + expect(followRename('b.ts', moves)).toBe('c.ts'); + }); + + // A review can span several commits, and a file can be moved twice across them. + it('follows a chain of them', () => { + 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-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/git/package.json b/packages/git/package.json index b2ee0a6..49c0f0d 100644 --- a/packages/git/package.json +++ b/packages/git/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/git", - "version": "0.9.11", + "version": "0.9.12", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/github/package.json b/packages/github/package.json index 13cfeaf..0cccdbb 100644 --- a/packages/github/package.json +++ b/packages/github/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/github", - "version": "0.9.11", + "version": "0.9.12", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/parser/package.json b/packages/parser/package.json index 3ced611..06f5a6e 100644 --- a/packages/parser/package.json +++ b/packages/parser/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/parser", - "version": "0.9.11", + "version": "0.9.12", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/ui/package.json b/packages/ui/package.json index 0943678..6e7123c 100644 --- a/packages/ui/package.json +++ b/packages/ui/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/ui", - "version": "0.9.11", + "version": "0.9.12", "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/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']); + }); +}); From aece3a5b4c86f60306466360d565024b2b89c8ef Mon Sep 17 00:00:00 2001 From: "Fredrik Liljegren (Claude Code Claude Opus 5)" Date: Wed, 26 Aug 2026 12:27:25 +0200 Subject: [PATCH 3/3] fix: ask git for the rename, not for the whole diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review. `donors` counts an unfinished review run, and `carryReviewRun` never finishes the donor's, so a donor keeps qualifying for as long as a review is open — and `/api/info` polls every five seconds. Behind that I had put a full `git diff` and a `parseDiff`. `--name-status --diff-filter=R` answers the same question in one line. Copies are out. git only reports them under `-C`, which is not asked for, so the branch was unreachable — and wrong if it ever ran, since a copy leaves the original where it is. v0.9.13. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018PkYQzbsnMihHesafWvXKs --- package-lock.json | 10 ++++---- packages/cli/package.json | 2 +- packages/cli/src/renames.ts | 23 +++++++---------- packages/cli/src/session.ts | 8 +++--- packages/cli/tests/renames.test.ts | 40 ++++++++++++++++-------------- packages/git/package.json | 2 +- packages/git/src/diff.ts | 8 ++++++ packages/git/src/index.ts | 2 +- packages/github/package.json | 2 +- packages/parser/package.json | 2 +- packages/ui/package.json | 2 +- 11 files changed, 53 insertions(+), 48 deletions(-) diff --git a/package-lock.json b/package-lock.json index d857314..a55a67b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8452,7 +8452,7 @@ }, "packages/cli": { "name": "diffity", - "version": "0.9.12", + "version": "0.9.13", "license": "MIT", "dependencies": { "commander": "^14.0.3", @@ -8476,7 +8476,7 @@ }, "packages/git": { "name": "@diffity/git", - "version": "0.9.12", + "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.12", + "version": "0.9.13", "dependencies": { "@diffity/parser": "*" }, @@ -8497,7 +8497,7 @@ }, "packages/parser": { "name": "@diffity/parser", - "version": "0.9.12", + "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.12", + "version": "0.9.13", "dependencies": { "@react-router/node": "^7.13.2", "@tailwindcss/vite": "^4.2.1", diff --git a/packages/cli/package.json b/packages/cli/package.json index de52228..90e91ca 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -1,6 +1,6 @@ { "name": "diffity", - "version": "0.9.12", + "version": "0.9.13", "description": "GitHub-style git diff viewer in the browser", "type": "module", "bin": { diff --git a/packages/cli/src/renames.ts b/packages/cli/src/renames.ts index 3a0ee7c..56c2ad5 100644 --- a/packages/cli/src/renames.ts +++ b/packages/cli/src/renames.ts @@ -1,26 +1,21 @@ -interface FileLike { - status: string; - oldPath: string; - newPath: string; -} - /** - * Where a file went, for the files git says moved. + * 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. * - * Taken from git's own rename detection rather than by matching content, so a thread only ever - * moves to a file git already said is the same file. + * `--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(files: FileLike[]): Map { +export function renamedPaths(nameStatus: string): Map { const moves = new Map(); - for (const file of files) { - if (file.status !== 'renamed' && file.status !== 'copied') continue; - if (!file.oldPath || !file.newPath || file.oldPath === file.newPath) continue; - moves.set(file.oldPath, file.newPath); + 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; diff --git a/packages/cli/src/session.ts b/packages/cli/src/session.ts index 072b08e..8ad4ae8 100644 --- a/packages/cli/src/session.ts +++ b/packages/cli/src/session.ts @@ -1,8 +1,7 @@ import { randomUUID } from 'node:crypto'; import { readFileSync, writeFileSync } from 'node:fs'; import { join } from 'node:path'; -import { getHeadHash, getDiffityDir, getRepoRoot, getCurrentBranch, getDiff, WORKING_TREE_REFS } from '@diffity/git'; -import { parseDiff } from '@diffity/parser'; +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'; @@ -112,6 +111,8 @@ export function findOrCreateSession(ref: string): Session { 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); } @@ -280,8 +281,7 @@ function followRenamesForSession(sessionId: string, donorIds: string[], toHead: for (const from of donorHeads(donorIds)) { if (from === toHead) continue; try { - // `-M` explicitly rather than relying on the default, which `diff.renames = false` turns off. - for (const [before, after] of renamedPaths(parseDiff(getDiff(['-M', from, toHead])).files)) { + for (const [before, after] of renamedPaths(getRenameStatus(from, toHead))) { moves.set(before, after); } } catch { diff --git a/packages/cli/tests/renames.test.ts b/packages/cli/tests/renames.test.ts index dac588a..8807892 100644 --- a/packages/cli/tests/renames.test.ts +++ b/packages/cli/tests/renames.test.ts @@ -1,28 +1,34 @@ import { describe, it, expect } from 'vitest'; import { renamedPaths, followRename } from '../src/renames.js'; -const file = (status: string, oldPath: string, newPath: string) => ({ status, oldPath, newPath }); - describe('renamedPaths', () => { - it('takes what git called a rename', () => { - const moves = renamedPaths([file('renamed', 'scripts/medical/const.ts', 'scripts/llm/const.ts')]); + 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 copy too, since the old path may be gone either way', () => { - expect(renamedPaths([file('copied', 'a.ts', 'b.ts')]).get('a.ts')).toBe('b.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 files that did not move', () => { - const moves = renamedPaths([ - file('modified', 'a.ts', 'a.ts'), - file('added', '', 'b.ts'), - file('deleted', 'c.ts', ''), - file('renamed', 'd.ts', 'd.ts'), - ]); + it('ignores everything that is not a rename', () => { + expect(renamedPaths('M\ta.ts\nA\tb.ts\nD\tc.ts\n').size).toBe(0); + }); - expect(moves.size).toBe(0); + it('is empty on empty output', () => { + expect(renamedPaths('').size).toBe(0); + expect(renamedPaths('\n\n').size).toBe(0); }); }); @@ -33,12 +39,8 @@ describe('followRename', () => { expect(followRename('untouched.ts', moves)).toBe('untouched.ts'); }); - it('follows a rename', () => { - expect(followRename('b.ts', moves)).toBe('c.ts'); - }); - // A review can span several commits, and a file can be moved twice across them. - it('follows a chain of them', () => { + it('follows a chain', () => { expect(followRename('a.ts', moves)).toBe('c.ts'); }); diff --git a/packages/git/package.json b/packages/git/package.json index 49c0f0d..dc4d82c 100644 --- a/packages/git/package.json +++ b/packages/git/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/git", - "version": "0.9.12", + "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 0cccdbb..439a461 100644 --- a/packages/github/package.json +++ b/packages/github/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/github", - "version": "0.9.12", + "version": "0.9.13", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/parser/package.json b/packages/parser/package.json index 06f5a6e..77d00aa 100644 --- a/packages/parser/package.json +++ b/packages/parser/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/parser", - "version": "0.9.12", + "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 6e7123c..1ac3bf2 100644 --- a/packages/ui/package.json +++ b/packages/ui/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/ui", - "version": "0.9.12", + "version": "0.9.13", "type": "module", "private": true, "scripts": {