Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion packages/cli/package.json
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
{
"name": "diffity",
"version": "0.9.7",
"version": "0.9.9",
"description": "GitHub-style git diff viewer in the browser",
"type": "module",
"bin": {
Expand Down
3 changes: 3 additions & 0 deletions packages/cli/src/db.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -149,6 +149,9 @@ function migrateDb(db: DatabaseSync): void {
// code that is there now?" — so the review and the commit it went out against are kept too.
addColumn(db, 'comment_threads', 'submitted_review_url', 'TEXT');
addColumn(db, 'comment_threads', 'submitted_head_sha', 'TEXT');
// The wording that went out. Amending a finding rewrites its body in place, so without this there
// is nothing left to recognise the forge's copy of it by.
addColumn(db, 'comment_threads', 'submitted_body', 'TEXT');
}

function addColumn(db: DatabaseSync, table: string, column: string, type: string): void {
Expand Down
59 changes: 59 additions & 0 deletions packages/cli/src/github-resolution.ts
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
export interface RemoteThreadState {
filePath: string;
side: 'old' | 'new';
/** Null once GitHub marks the thread outdated, so it cannot be part of the identity. */
endLine: number | null;
body: string;
isResolved: boolean;
}

interface LocalThreadLike {
id: string;
filePath: string;
side: string;
endLine: number;
status: string;
submittedAt?: string | null;
submittedBody?: string | null;
comments: { body: string }[];
}

/**
* Which local threads the forge now considers settled.
*
* Only threads we sent are considered: a thread that was never posted cannot have been resolved by
* the author, and matching one to a remote thread that merely looks like it would resolve a finding
* nobody has seen.
*
* Matched on file and wording rather than on line. GitHub nulls a thread's line once it goes
* outdated, which is the state most resolved threads are in by the time anyone looks, so a line in
* the key means the sync quietly does nothing on exactly the threads it exists for. Two findings
* with identical wording in one file would both resolve together; a missed resolution leaves a
* thread open, which is the cheaper way to be wrong.
*
* The wording compared is the one that was sent, not the one held now: amending rewrites the body
* here and leaves the forge showing the old text. Threads sent before that was recorded fall back
* to their current bodies, which is what they had at the time anyway.
*/
export function threadsResolvedRemotely(
local: LocalThreadLike[],
remote: RemoteThreadState[],
): string[] {
const resolvedRemotely = remote.filter(state => state.isResolved);

return local
.filter(thread => thread.submittedAt && thread.status === 'open')
.filter(thread =>
resolvedRemotely.some(
state =>
state.filePath === thread.filePath
&& state.side === thread.side
&& wordingSent(thread).includes(state.body),
),
)
.map(thread => thread.id);
}

function wordingSent(thread: LocalThreadLike): string[] {
return thread.submittedBody ? [thread.submittedBody] : thread.comments.map(comment => comment.body);
}
24 changes: 18 additions & 6 deletions packages/cli/src/server.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -43,6 +43,7 @@ import {
fetchDetails as fetchGitHubDetails,
createReview as createGitHubReview,
pullComments as pullGitHubComments,
pullThreadState as pullGitHubThreadState,
type PrComment,
type ReviewEvent,
} from '@diffity/github';
Expand All@@ -59,7 +60,8 @@ import { computeDiffFingerprint } from './fingerprint.js';
import { parseDiffStatFiles } from './diff-stat.js';
import { parseDiffStatSummary } from './diff-stat.js';
import { getReviewRun } from './review-run.js';
import { createThread, addReply, getThreadsForSession, markThreadsSubmitted } from './threads.js';
import { createThread, addReply, getThreadsForSession, markThreadsSubmitted, updateThreadStatus } from './threads.js';
import { threadsResolvedRemotely } from './github-resolution.js';
import { handleReviewRoute } from './review-routes.js';
import { handleTourRoute } from './tour-routes.js';
import { sendJson, sendError, readBody } from './http-utils.js';
Expand DownExpand Up@@ -700,10 +702,14 @@ export function startServer(options: ServerOptions): Promise<ServerResult> {
details.headSha,
{ event, body: summary, comments },
);
markThreadsSubmitted(result.submittedThreadIds, {
reviewUrl: result.reviewUrl,
headSha: details.headSha,
});
const sentBodies = new Map(comments.map(comment => [comment.threadId, comment.body]));
markThreadsSubmitted(
result.submittedThreadIds.map(threadId => ({ threadId, body: sentBodies.get(threadId) })),
{
reviewUrl: result.reviewUrl,
headSha: details.headSha,
},
);
sendJson(res, result);
return;
}
Expand DownExpand Up@@ -738,6 +744,12 @@ export function startServer(options: ServerOptions): Promise<ServerResult> {
const remoteThreads = pullGitHubComments(githubRemote.owner, githubRemote.repo, details.prNumber);
const localThreads = getThreadsForSession(sid);

const remoteState = pullGitHubThreadState(githubRemote.owner, githubRemote.repo, details.prNumber);
const settled = remoteState ? threadsResolvedRemotely(localThreads, remoteState) : [];
for (const threadId of settled) {
updateThreadStatus(threadId, 'resolved');
}

let pulled = 0;
let skipped = 0;
for (const rt of remoteThreads) {
Expand DownExpand Up@@ -766,7 +778,7 @@ export function startServer(options: ServerOptions): Promise<ServerResult> {
}
pulled++;
}
sendJson(res, { pulled, skipped });
sendJson(res, { pulled, skipped, resolved: settled.length, resolutionUnavailable: remoteState === null });
return;
}

Expand Down
27 changes: 20 additions & 7 deletions packages/cli/src/threads.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -38,12 +38,15 @@ export interface Thread {
submittedAt: string | null;
submittedReviewUrl: string | null;
submittedHeadSha: string | null;
/** The body as it was sent, which an amendment here does not change. */
submittedBody: string | null;
comments: ThreadComment[];
}

interface ThreadRow {
submitted_at?: string | null;
submitted_review_url?: string | null;
submitted_body?: string | null;
submitted_head_sha?: string | null;
id: string;
session_id: string;
Expand DownExpand Up@@ -85,6 +88,7 @@ function rowToThread(row: ThreadRow, comments: ThreadComment[]): Thread {
updatedAt: row.updated_at,
submittedAt: row.submitted_at ?? null,
submittedReviewUrl: row.submitted_review_url ?? null,
submittedBody: row.submitted_body ?? null,
submittedHeadSha: row.submitted_head_sha ?? null,
comments,
};
Expand DownExpand Up@@ -133,20 +137,28 @@ export interface SubmittedIn {
headSha?: string | null;
}

export function markThreadsSubmitted(threadIds: string[], submittedIn: SubmittedIn = {}): void {
if (threadIds.length === 0) {
export function markThreadsSubmitted(
sent: (string | { threadId: string; body?: string })[],
submittedIn: SubmittedIn = {},
): void {
if (sent.length === 0) {
return;
}

const db = getDb();
const placeholders = threadIds.map(() => '?').join(', ');
db.prepare(
const statement = db.prepare(
`UPDATE comment_threads
SET submitted_at = datetime('now'),
submitted_review_url = ?,
submitted_head_sha = ?
WHERE id IN (${placeholders})`,
).run(submittedIn.reviewUrl ?? null, submittedIn.headSha ?? null, ...threadIds);
submitted_head_sha = ?,
submitted_body = COALESCE(?, submitted_body)
WHERE id = ?`,
);

for (const entry of sent) {
const { threadId, body } = typeof entry === 'string' ? { threadId: entry, body: undefined } : entry;
statement.run(submittedIn.reviewUrl ?? null, submittedIn.headSha ?? null, body ?? null, threadId);
}
}

export function updateThreadLines(threadId: string, startLine: number, endLine: number): void {
Expand DownExpand Up@@ -197,6 +209,7 @@ export function createThread(
updatedAt: now,
submittedAt: null,
submittedReviewUrl: null,
submittedBody: null,
submittedHeadSha: null,
comments: [{
id: commentId,
Expand Down
89 changes: 89 additions & 0 deletions packages/cli/tests/github-resolution.test.ts
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
import { describe, it, expect } from 'vitest';
import { threadsResolvedRemotely } from '../src/github-resolution.js';
import type { RemoteThreadState } from '../src/github-resolution.js';

function local(over: Partial<Parameters<typeof threadsResolvedRemotely>[0][number]> = {}) {
return {
id: 't1',
filePath: 'src/a.ts',
side: 'new',
endLine: 54,
status: 'open',
submittedAt: '2026-08-24T15:37:00Z',
comments: [{ body: 'P2: the finding' }],
...over,
};
}

function remote(over: Partial<RemoteThreadState> = {}): RemoteThreadState {
return {
filePath: 'src/a.ts',
side: 'new',
endLine: 54,
body: 'P2: the finding',
isResolved: true,
...over,
};
}

describe('threadsResolvedRemotely', () => {
it('takes a sent thread the author has resolved', () => {
expect(threadsResolvedRemotely([local()], [remote()])).toEqual(['t1']);
});

it('leaves a thread the author has not resolved', () => {
expect(threadsResolvedRemotely([local()], [remote({ isResolved: false })])).toEqual([]);
});

it('leaves a thread that was never sent', () => {
expect(threadsResolvedRemotely([local({ submittedAt: null })], [remote()])).toEqual([]);
});

it('leaves a thread already resolved here, so nothing is written twice', () => {
expect(threadsResolvedRemotely([local({ status: 'resolved' })], [remote()])).toEqual([]);
});

it('does not match a different side or file', () => {
expect(threadsResolvedRemotely([local()], [remote({ side: 'old' })])).toEqual([]);
expect(threadsResolvedRemotely([local()], [remote({ filePath: 'src/b.ts' })])).toEqual([]);
});

// GitHub nulls the line once a thread goes outdated, and an outdated thread is the usual state of
// a resolved one. Keyed on the line, this sync would do nothing on the threads it exists for.
it('matches an outdated thread, which has no line left', () => {
expect(threadsResolvedRemotely([local()], [remote({ endLine: null })])).toEqual(['t1']);
});

it('matches when the code moved under the thread', () => {
expect(threadsResolvedRemotely([local({ endLine: 54 })], [remote({ endLine: 91 })])).toEqual(['t1']);
});

// Two findings can sit on one line, and resolving one must not resolve the other.
it('tells two findings on the same line apart by their body', () => {
const threads = [local({ id: 'a' }), local({ id: 'b', comments: [{ body: 'P3: the other one' }] })];

expect(threadsResolvedRemotely(threads, [remote()])).toEqual(['a']);
});

// Amending rewrites the body in place, so the wording that went out survives only here. Without
// it an amended finding stops matching, which since #32 is most of the ones that carry an answer.
it('matches an amended finding on the wording that was sent', () => {
const amended = local({
comments: [{ body: 'P2: the finding, amended to carry the answer' }],
submittedBody: 'P2: the finding',
});

expect(threadsResolvedRemotely([amended], [remote()])).toEqual(['t1']);
});

it('does not match a thread whose sent wording was something else entirely', () => {
const other = local({ comments: [{ body: 'P2: the finding' }], submittedBody: 'P3: unrelated' });

expect(threadsResolvedRemotely([other], [remote()])).toEqual([]);
});

// Everything sent before the column existed has no record of its wording.
it('falls back to the current wording when none was recorded', () => {
expect(threadsResolvedRemotely([local({ submittedBody: null })], [remote()])).toEqual(['t1']);
});
});
2 changes: 1 addition & 1 deletion packages/git/package.json
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
{
"name": "@diffity/git",
"version": "0.9.7",
"version": "0.9.9",
"private": true,
"type": "module",
"main": "./dist/index.js",
Expand Down
2 changes: 1 addition & 1 deletion packages/github/package.json
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
{
"name": "@diffity/github",
"version": "0.9.7",
"version": "0.9.9",
"private": true,
"type": "module",
"main": "./dist/index.js",
Expand Down
3 changes: 2 additions & 1 deletion packages/github/src/index.ts
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
export type { GitHubRemote, GitHubDetails, PrBase, PrComment, PrReview, PulledThread, ReviewEvent, ReviewResult, ReviewSubmission } from './types.js';
export { detectRemote, fetchDetails, isCliInstalled, isAuthenticated } from './detection.js';
export { getComments, getCommentCount, pullComments, createReview } from './pr.js';
export { getComments, getCommentCount, pullComments, pullThreadState, createReview } from './pr.js';
export type { RemoteThreadState } from './pr.js';
export { getReviews, parseReviews } from './reviews.js';
export { commentableLines, isAlreadyCommented } from './comment-targets.js';
export { isGitHubPrUrl, parseGitHubPrUrl, checkoutPr, getPrBase, parsePrBase } from './pr-url.js';
Loading