diff --git a/package-lock.json b/package-lock.json index 3d488e1..dc60121 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8452,7 +8452,7 @@ }, "packages/cli": { "name": "diffity", - "version": "0.9.7", + "version": "0.9.9", "license": "MIT", "dependencies": { "commander": "^14.0.3", @@ -8476,7 +8476,7 @@ }, "packages/git": { "name": "@diffity/git", - "version": "0.9.7", + "version": "0.9.9", "devDependencies": { "@types/node": "^25.5.0", "typescript": "^5.9.3", @@ -8485,7 +8485,7 @@ }, "packages/github": { "name": "@diffity/github", - "version": "0.9.7", + "version": "0.9.9", "dependencies": { "@diffity/parser": "*" }, @@ -8497,7 +8497,7 @@ }, "packages/parser": { "name": "@diffity/parser", - "version": "0.9.7", + "version": "0.9.9", "devDependencies": { "typescript": "^5.9.3", "vitest": "^4.1.0" @@ -8505,7 +8505,7 @@ }, "packages/ui": { "name": "@diffity/ui", - "version": "0.9.7", + "version": "0.9.9", "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 79cc535..4fa0ad5 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -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": { diff --git a/packages/cli/src/db.ts b/packages/cli/src/db.ts index 1feccbf..2bc1bbe 100644 --- a/packages/cli/src/db.ts +++ b/packages/cli/src/db.ts @@ -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 { diff --git a/packages/cli/src/github-resolution.ts b/packages/cli/src/github-resolution.ts new file mode 100644 index 0000000..47cfc0c --- /dev/null +++ b/packages/cli/src/github-resolution.ts @@ -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); +} diff --git a/packages/cli/src/server.ts b/packages/cli/src/server.ts index 89e88b2..0610077 100644 --- a/packages/cli/src/server.ts +++ b/packages/cli/src/server.ts @@ -43,6 +43,7 @@ import { fetchDetails as fetchGitHubDetails, createReview as createGitHubReview, pullComments as pullGitHubComments, + pullThreadState as pullGitHubThreadState, type PrComment, type ReviewEvent, } from '@diffity/github'; @@ -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'; @@ -700,10 +702,14 @@ export function startServer(options: ServerOptions): Promise { 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; } @@ -738,6 +744,12 @@ export function startServer(options: ServerOptions): Promise { 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) { @@ -766,7 +778,7 @@ export function startServer(options: ServerOptions): Promise { } pulled++; } - sendJson(res, { pulled, skipped }); + sendJson(res, { pulled, skipped, resolved: settled.length, resolutionUnavailable: remoteState === null }); return; } diff --git a/packages/cli/src/threads.ts b/packages/cli/src/threads.ts index d98a07e..94877e0 100644 --- a/packages/cli/src/threads.ts +++ b/packages/cli/src/threads.ts @@ -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; @@ -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, }; @@ -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 { @@ -197,6 +209,7 @@ export function createThread( updatedAt: now, submittedAt: null, submittedReviewUrl: null, + submittedBody: null, submittedHeadSha: null, comments: [{ id: commentId, diff --git a/packages/cli/tests/github-resolution.test.ts b/packages/cli/tests/github-resolution.test.ts new file mode 100644 index 0000000..ec36343 --- /dev/null +++ b/packages/cli/tests/github-resolution.test.ts @@ -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[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 { + 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']); + }); +}); diff --git a/packages/git/package.json b/packages/git/package.json index cc33dd8..855df98 100644 --- a/packages/git/package.json +++ b/packages/git/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/git", - "version": "0.9.7", + "version": "0.9.9", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/github/package.json b/packages/github/package.json index 606217c..bb789ab 100644 --- a/packages/github/package.json +++ b/packages/github/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/github", - "version": "0.9.7", + "version": "0.9.9", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/github/src/index.ts b/packages/github/src/index.ts index db20c4b..4e0abd4 100644 --- a/packages/github/src/index.ts +++ b/packages/github/src/index.ts @@ -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'; diff --git a/packages/github/src/pr.ts b/packages/github/src/pr.ts index 82474e2..ad7f069 100644 --- a/packages/github/src/pr.ts +++ b/packages/github/src/pr.ts @@ -52,6 +52,85 @@ interface GitHubCommentRaw { created_at: string; } +export interface RemoteThreadState { + filePath: string; + side: 'old' | 'new'; + endLine: number | null; + body: string; + isResolved: boolean; +} + +const REVIEW_THREADS_QUERY = `query($owner:String!,$repo:String!,$number:Int!){ + repository(owner:$owner,name:$repo){ + pullRequest(number:$number){ + reviewThreads(first:100){ + nodes{ + isResolved + line + originalLine + diffSide + path + comments(first:1){ nodes{ body } } + } + } + } + } +}`; + +/** + * Whether the author has ticked a thread off. REST does not carry it — resolution lives on + * `PullRequestReviewThread`, which is GraphQL only — so this is a second call rather than a field + * on the comments we already fetch. + * + * One page. A review with more than a hundred threads reports the first hundred, which is wrong in + * a way that only under-reports: a thread we do not see is left open here, never wrongly closed. + * + * `null` means the question could not be asked — an expired token, a rate limit, a schema change — + * as opposed to `[]`, which means it was asked and nothing came back. The two look identical to a + * reader otherwise, and this whole change exists because a thread was quiet about what it knew. + */ +export function pullThreadState(owner: string, repo: string, prNumber: number): RemoteThreadState[] | null { + try { + const json = gh([ + 'api', 'graphql', + '-f', `query=${REVIEW_THREADS_QUERY}`, + '-F', `owner=${owner}`, + '-F', `repo=${repo}`, + '-F', `number=${prNumber}`, + ]); + if (!json) return null; + + const data = JSON.parse(json) as { + data?: { repository?: { pullRequest?: { reviewThreads?: { nodes?: RawReviewThread[] } } } }; + }; + const nodes = data.data?.repository?.pullRequest?.reviewThreads?.nodes ?? []; + + return nodes.flatMap(node => { + const body = node.comments?.nodes?.[0]?.body; + if (!body || !node.path) return []; + return [{ + filePath: node.path, + side: node.diffSide === 'LEFT' ? 'old' as const : 'new' as const, + // Null once the thread goes outdated, which is why it is not part of the identity. + endLine: node.line ?? node.originalLine ?? null, + body, + isResolved: !!node.isResolved, + }]; + }); + } catch { + return null; + } +} + +interface RawReviewThread { + isResolved: boolean; + line: number | null; + originalLine: number | null; + diffSide: string; + path: string; + comments?: { nodes?: { body: string }[] }; +} + export function pullComments(owner: string, repo: string, prNumber: number): PulledThread[] { try { const json = gh([ diff --git a/packages/parser/package.json b/packages/parser/package.json index 9dd16f7..37b1554 100644 --- a/packages/parser/package.json +++ b/packages/parser/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/parser", - "version": "0.9.7", + "version": "0.9.9", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/ui/package.json b/packages/ui/package.json index 0350035..5455ab5 100644 --- a/packages/ui/package.json +++ b/packages/ui/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/ui", - "version": "0.9.7", + "version": "0.9.9", "type": "module", "private": true, "scripts": { diff --git a/packages/ui/src/components/comments/thread-card.tsx b/packages/ui/src/components/comments/thread-card.tsx index fcf96d8..7e96d59 100644 --- a/packages/ui/src/components/comments/thread-card.tsx +++ b/packages/ui/src/components/comments/thread-card.tsx @@ -1,6 +1,7 @@ import { useState } from 'react'; import type { CommentThread as CommentThreadType } from './types'; import { isThreadResolved } from './types'; +import { submittedLabel } from '../../lib/submitted-marker'; import { CommentBubble } from './comment-bubble'; import { CommentForm } from './comment-form'; import { TrashIcon } from '../icons/trash-icon'; @@ -41,12 +42,18 @@ export function ThreadCard(props: ThreadCardProps) { onAskReply, onActReply, askIsHeard,} = props; const [showReply, setShowReply] = useState(false); const resolved = isThreadResolved(thread); + const sentLabel = submittedLabel(thread.submittedAt); return (
{headerLeft} + {sentLabel && ( + + {sentLabel} + + )}
{onResolve && onUnresolve && ( diff --git a/packages/ui/src/components/layout/github-dialog.tsx b/packages/ui/src/components/layout/github-dialog.tsx index 3d0ce2a..9701d50 100644 --- a/packages/ui/src/components/layout/github-dialog.tsx +++ b/packages/ui/src/components/layout/github-dialog.tsx @@ -20,6 +20,7 @@ import { summaryFromGeneralThreads, threadToPayload, } from '../../lib/review-submission'; +import { pullOutcome } from '../../lib/pull-outcome'; dayjs.extend(relativeTime); @@ -156,10 +157,9 @@ export function GitHubDialog(props: GitHubDialogProps) { setPulling(true); try { const result = await pullCommentsFromGitHub(sessionId); - if (result.pulled === 0 && result.skipped > 0) { - toast.info('All GitHub comments already exist locally'); - } else if (result.pulled > 0) { - toast.success(`Pulled ${result.pulled} comment${result.pulled !== 1 ? 's' : ''} from PR`); + const outcome = pullOutcome(result); + toast[outcome.kind](outcome.message); + if (outcome.refresh) { onPulled(); } } catch (err) { diff --git a/packages/ui/src/hooks/use-comment-actions.ts b/packages/ui/src/hooks/use-comment-actions.ts index 75309ec..a4704f2 100644 --- a/packages/ui/src/hooks/use-comment-actions.ts +++ b/packages/ui/src/hooks/use-comment-actions.ts @@ -1,7 +1,9 @@ import { useCallback } from 'react'; import { useQueryClient } from '@tanstack/react-query'; -import type { CommentAuthor, CommentSide } from '../components/comments/types'; +import { toast } from 'sonner'; +import type { CommentAuthor, CommentSide, CommentThread } from '../components/comments/types'; import * as api from '../lib/api'; +import { localResolveNotice } from '../lib/submitted-marker'; export function useCommentActions(sessionId: string | null, enabled: boolean) { const queryClient = useQueryClient(); @@ -37,10 +39,16 @@ export function useCommentActions(sessionId: string | null, enabled: boolean) { if (!enabled) { return; } + const threads = queryClient.getQueryData(['threads', sessionId]); + const notice = localResolveNotice(threads?.find(thread => thread.id === threadId)?.submittedAt); + api.updateThreadStatus(threadId, 'resolved').then(() => { invalidateThreads(); + if (notice) { + toast.info(notice); + } }); - }, [enabled, invalidateThreads]); + }, [enabled, invalidateThreads, queryClient, sessionId]); const unresolveThread = useCallback((threadId: string) => { if (!enabled) { @@ -55,10 +63,19 @@ export function useCommentActions(sessionId: string | null, enabled: boolean) { if (!enabled) { return; } + const threads = queryClient.getQueryData(['threads', sessionId]); + const notice = localResolveNotice( + threads?.find(thread => thread.id === threadId)?.submittedAt, + 'dismissed', + ); + api.updateThreadStatus(threadId, 'dismissed').then(() => { invalidateThreads(); + if (notice) { + toast.info(notice); + } }); - }, [enabled, invalidateThreads]); + }, [enabled, invalidateThreads, queryClient, sessionId]); const editComment = useCallback((commentId: string, body: string) => { if (!enabled) { diff --git a/packages/ui/src/lib/api.ts b/packages/ui/src/lib/api.ts index 449cb99..6e82218 100644 --- a/packages/ui/src/lib/api.ts +++ b/packages/ui/src/lib/api.ts @@ -314,6 +314,8 @@ export function createReviewOnGitHub(review: { export interface PullCommentsResult { pulled: number; + resolved: number; + resolutionUnavailable?: boolean; skipped: number; } diff --git a/packages/ui/src/lib/pull-outcome.ts b/packages/ui/src/lib/pull-outcome.ts new file mode 100644 index 0000000..eefa00e --- /dev/null +++ b/packages/ui/src/lib/pull-outcome.ts @@ -0,0 +1,42 @@ +export interface PullCounts { + pulled: number; + skipped: number; + resolved: number; + /** The forge could not be asked which threads are resolved, as opposed to answering "none". */ + resolutionUnavailable?: boolean; +} + +export interface PullOutcome { + kind: 'success' | 'info'; + message: string; + /** Whether anything changed here, and the page has to be re-read to show it. */ + refresh: boolean; +} + +function count(n: number, noun: string): string { + return `${n} ${noun}${n === 1 ? '' : 's'}`; +} + +export function pullOutcome({ pulled, skipped, resolved, resolutionUnavailable }: PullCounts): PullOutcome { + const parts: string[] = []; + if (pulled > 0) parts.push(`Pulled ${count(pulled, 'comment')}`); + if (resolved > 0) parts.push(`${count(resolved, 'finding')} resolved on the pull request`); + + if (resolutionUnavailable) { + parts.push('could not read which are resolved'); + } + + if (parts.length === 0) { + return { + kind: 'info', + message: skipped > 0 ? 'Nothing new — every comment is already here' : 'Nothing to pull', + refresh: false, + }; + } + + return { + kind: resolutionUnavailable ? 'info' : 'success', + message: parts.join(', '), + refresh: pulled > 0 || resolved > 0, + }; +} diff --git a/packages/ui/src/lib/submitted-marker.ts b/packages/ui/src/lib/submitted-marker.ts new file mode 100644 index 0000000..7bc6fbb --- /dev/null +++ b/packages/ui/src/lib/submitted-marker.ts @@ -0,0 +1,41 @@ +const MONTHS = ['Jan', 'Feb', 'Mar', 'Apr', 'May', 'Jun', 'Jul', 'Aug', 'Sep', 'Oct', 'Nov', 'Dec']; + +function clockTime(date: Date): string { + return `${String(date.getHours()).padStart(2, '0')}:${String(date.getMinutes()).padStart(2, '0')}`; +} + +function daysApart(from: Date, to: Date): number { + const startOf = (d: Date) => new Date(d.getFullYear(), d.getMonth(), d.getDate()).getTime(); + return Math.round((startOf(to) - startOf(from)) / 86_400_000); +} + +/** + * A time of day rather than "19h ago": the question this answers is whether the author has seen it + * yet, which is a question about when it landed, not about how long ago that was. + */ +export function submittedLabel(submittedAt: string | null | undefined, now = new Date()): string | null { + if (!submittedAt) return null; + + const date = new Date(submittedAt); + if (Number.isNaN(date.getTime())) return null; + + const days = daysApart(date, now); + if (days === 0) return `Posted to GitHub ${clockTime(date)}`; + if (days === 1) return `Posted to GitHub yesterday ${clockTime(date)}`; + + const day = `${date.getDate()} ${MONTHS[date.getMonth()]}`; + const withYear = date.getFullYear() === now.getFullYear() ? day : `${day} ${date.getFullYear()}`; + return `Posted to GitHub ${withYear} ${clockTime(date)}`; +} + +/** + * Resolving here does not resolve there. Only worth saying about a finding that was posted — for + * anything else, local is the only place it could be resolved. + */ +export function localResolveNotice( + submittedAt: string | null | undefined, + action: 'resolved' | 'dismissed' = 'resolved', +): string | null { + if (!submittedAt) return null; + return `${action === 'resolved' ? 'Resolved' : 'Dismissed'} here only — the thread on the pull request stays open`; +} diff --git a/packages/ui/tests/pull-outcome.test.ts b/packages/ui/tests/pull-outcome.test.ts new file mode 100644 index 0000000..a3345ba --- /dev/null +++ b/packages/ui/tests/pull-outcome.test.ts @@ -0,0 +1,50 @@ +import { describe, it, expect } from 'vitest'; +import { pullOutcome } from '../src/lib/pull-outcome'; + +describe('pullOutcome', () => { + it('reports what came back', () => { + expect(pullOutcome({ pulled: 2, skipped: 0, resolved: 0 }).message).toBe('Pulled 2 comments'); + expect(pullOutcome({ pulled: 1, skipped: 0, resolved: 0 }).message).toBe('Pulled 1 comment'); + }); + + // The reason this is not folded into the pulled count: nothing arrived, but something changed. + it('reports resolutions on their own, and asks for a refresh', () => { + const outcome = pullOutcome({ pulled: 0, skipped: 4, resolved: 1 }); + + expect(outcome.message).toBe('1 finding resolved on the pull request'); + expect(outcome.refresh).toBe(true); + expect(outcome.kind).toBe('success'); + }); + + it('reports both together', () => { + expect(pullOutcome({ pulled: 2, skipped: 1, resolved: 3 }).message) + .toBe('Pulled 2 comments, 3 findings resolved on the pull request'); + }); + + it('does not ask for a refresh when nothing changed', () => { + const outcome = pullOutcome({ pulled: 0, skipped: 5, resolved: 0 }); + + expect(outcome.refresh).toBe(false); + expect(outcome.kind).toBe('info'); + expect(outcome.message).toBe('Nothing new — every comment is already here'); + }); + + it('has something to say when the pull request had nothing at all', () => { + expect(pullOutcome({ pulled: 0, skipped: 0, resolved: 0 }).message).toBe('Nothing to pull'); + }); +}); + +describe('when the forge could not be asked about resolution', () => { + it('says so rather than implying nothing was resolved', () => { + const outcome = pullOutcome({ pulled: 0, skipped: 3, resolved: 0, resolutionUnavailable: true }); + + expect(outcome.message).toBe('could not read which are resolved'); + expect(outcome.kind).toBe('info'); + expect(outcome.refresh).toBe(false); + }); + + it('still reports what did arrive', () => { + expect(pullOutcome({ pulled: 2, skipped: 0, resolved: 0, resolutionUnavailable: true }).message) + .toBe('Pulled 2 comments, could not read which are resolved'); + }); +}); diff --git a/packages/ui/tests/submitted-marker-render.test.tsx b/packages/ui/tests/submitted-marker-render.test.tsx new file mode 100644 index 0000000..c156dd6 --- /dev/null +++ b/packages/ui/tests/submitted-marker-render.test.tsx @@ -0,0 +1,46 @@ +import { describe, it, expect, afterEach } from 'vitest'; +import { render, cleanup, screen } from '@testing-library/react'; +import { ThreadCard } from '../src/components/comments/thread-card'; +import type { CommentThread } from '../src/components/comments/types'; + +afterEach(cleanup); + +function thread(over: Partial = {}): CommentThread { + return { + id: 't1', + filePath: 'src/a.ts', + side: 'new', + startLine: 51, + endLine: 54, + status: 'open', + comments: [{ + id: 'c0', + author: { name: 'Agent', type: 'agent' }, + body: 'Do we need .allowAdditionalProperties() here?', + createdAt: new Date().toISOString(), + }], + ...over, + } as CommentThread; +} + +function show(t: CommentThread) { + render( + {}} onDeleteComment={() => {}} onDeleteThread={() => {}} />, + ); +} + +describe('the card says whether a finding has been sent', () => { + it('marks one that was, with the time it landed', () => { + const at = new Date(); + at.setHours(15, 37, 0, 0); + show(thread({ submittedAt: at.toISOString() })); + + expect(screen.getByTestId('submitted-marker').textContent).toBe('Posted to GitHub 15:37'); + }); + + it('says nothing on one that was not', () => { + show(thread()); + + expect(screen.queryByTestId('submitted-marker')).toBeNull(); + }); +}); diff --git a/packages/ui/tests/submitted-marker.test.ts b/packages/ui/tests/submitted-marker.test.ts new file mode 100644 index 0000000..92eded3 --- /dev/null +++ b/packages/ui/tests/submitted-marker.test.ts @@ -0,0 +1,50 @@ +import { describe, it, expect } from 'vitest'; +import { submittedLabel, localResolveNotice } from '../src/lib/submitted-marker'; + +const now = new Date(2026, 7, 25, 18, 4); + +describe('submittedLabel', () => { + it('says nothing for a thread that was never sent', () => { + expect(submittedLabel(null, now)).toBeNull(); + expect(submittedLabel(undefined, now)).toBeNull(); + }); + + it('gives the time for today', () => { + expect(submittedLabel(new Date(2026, 7, 25, 15, 37).toISOString(), now)) + .toBe('Posted to GitHub 15:37'); + }); + + it('names yesterday', () => { + expect(submittedLabel(new Date(2026, 7, 24, 15, 37).toISOString(), now)) + .toBe('Posted to GitHub yesterday 15:37'); + }); + + // Yesterday is a calendar day, not 24 hours: 23:50 last night is yesterday at 00:10 tonight. + it('counts yesterday by the calendar', () => { + expect(submittedLabel(new Date(2026, 7, 24, 23, 50).toISOString(), new Date(2026, 7, 25, 0, 10))) + .toBe('Posted to GitHub yesterday 23:50'); + }); + + it('gives a date further back, and the year once it is another one', () => { + expect(submittedLabel(new Date(2026, 7, 12, 9, 5).toISOString(), now)) + .toBe('Posted to GitHub 12 Aug 09:05'); + expect(submittedLabel(new Date(2025, 7, 12, 9, 5).toISOString(), now)) + .toBe('Posted to GitHub 12 Aug 2025 09:05'); + }); + + it('says nothing for a timestamp it cannot read', () => { + expect(submittedLabel('not a date', now)).toBeNull(); + }); +}); + +describe('localResolveNotice', () => { + it('warns on a finding that was posted', () => { + expect(localResolveNotice('2026-08-24T15:37:00Z')) + .toBe('Resolved here only — the thread on the pull request stays open'); + }); + + it('says nothing about one that was never posted, which has nowhere else to be resolved', () => { + expect(localResolveNotice(null)).toBeNull(); + expect(localResolveNotice(undefined)).toBeNull(); + }); +});