From 457747163a6932b4149ae3a27e76f58377027e73 Mon Sep 17 00:00:00 2001 From: syn Date: Sat, 18 Jul 2026 10:21:49 -0500 Subject: [PATCH 1/2] feat(review): add review map cursor foundation --- .changeset/bright-review-maps-guide.md | 5 + .plans/DEVIATIONS.md | 7 + .plans/DIFF_IMPROVEMENTS_PLAN.md | 15 +- .plans/PR_REVIEW_PERF_PLAN.md | 38 +- shared/review-navigation.ts | 505 ++++++++++++++++++ src/review-navigation.test.ts | 347 ++++++++++++ .../diff-viewer/FileTreePane.test.tsx | 144 +++++ web/src/features/diff-viewer/FileTreePane.tsx | 120 ++++- .../features/diff-viewer/MultiFileView.tsx | 4 + web/src/features/diff-viewer/types.ts | 11 + .../features/pr-review/GitHubPrReview.test.ts | 58 ++ web/src/features/pr-review/GitHubPrReview.tsx | 19 + .../features/pr-review/PrReviewDiffPane.tsx | 9 +- .../features/pr-review/review-view-model.ts | 103 +++- 14 files changed, 1359 insertions(+), 26 deletions(-) create mode 100644 .changeset/bright-review-maps-guide.md create mode 100644 shared/review-navigation.ts create mode 100644 src/review-navigation.test.ts create mode 100644 web/src/features/diff-viewer/FileTreePane.test.tsx diff --git a/.changeset/bright-review-maps-guide.md b/.changeset/bright-review-maps-guide.md new file mode 100644 index 00000000..6c1e5864 --- /dev/null +++ b/.changeset/bright-review-maps-guide.md @@ -0,0 +1,5 @@ +--- +'neondeck': patch +--- + +Show per-file PR review status in the changed-file tree and add a shared cross-file cursor foundation for files, hunks, threads, drafts, findings, and combined attention items. diff --git a/.plans/DEVIATIONS.md b/.plans/DEVIATIONS.md index 63eddc30..88b230a9 100644 --- a/.plans/DEVIATIONS.md +++ b/.plans/DEVIATIONS.md @@ -15,6 +15,13 @@ Use this format: - Follow-up: What remains, who/what should handle it, or `None`. ``` +## 2026-07-18 - Diff Review Performance Reconciliation + +- Roadmap item: Diff Improvements Plan / transition from Phase A to Phase B +- Decision: Mark the specialized PR review performance plan complete for now and unpause Phase B while explicitly deferring the production tree median (642 ms versus <500 ms), one-time cold local object fetch (4,978 ms versus <3,000 ms), and uncached review-thread surface/read latency (1,511 ms surface and 655 ms initial GitHub-backed read versus the warm path's <500 ms target). Retain the specialized plan in place instead of archiving it while those measured misses remain. +- Reason: Stable query identity, bounded immutable metadata reuse, active-patch prioritization, and bounded warm thread reuse delivered passing warm first-patch and thread medians and removed duplicate/abandoned work. The remaining misses are measured, isolated follow-ups that do not require overlapping changes in the Phase B review-map/cursor seam, but they must not be represented as passing or erased. +- Follow-up: Reprofile the production tree boot/query/render boundary; separate cold network object-fetch time from local metadata before changing refspecs or the <3-second budget; and evaluate uncached GitHub thread latency without weakening cancellation or mutation invalidation. Remeasure the retained immutable real PR before changing any budget, then archive `.plans/PR_REVIEW_PERF_PLAN.md` only after these deferrals are reconciled. + ## 2026-07-17 - Diff Review Phase A Sequencing - Roadmap item: Diff Improvements Plan / transition from Phase A to Phase B diff --git a/.plans/DIFF_IMPROVEMENTS_PLAN.md b/.plans/DIFF_IMPROVEMENTS_PLAN.md index 7cdc8262..beb96e9c 100644 --- a/.plans/DIFF_IMPROVEMENTS_PLAN.md +++ b/.plans/DIFF_IMPROVEMENTS_PLAN.md @@ -1,10 +1,10 @@ # Diff Improvements Plan -Status: active; Phase A implementation is complete in PR #143, Phase B is paused, and the specialized PR review performance workstream has resumed +Status: active; Phase A implementation is complete in PR #143, Phase B is active, and the specialized PR review performance workstream is complete for now with measured misses explicitly deferred -Progress note (2026-07-18): the specialized large-PR work now has real registered-PR measurements, stable review-thread identity, bounded local metadata reuse, active-patch priority, and a passing first-patch browser budget. Reconciliation after Phase A found that production tree visibility, review-thread visibility, and the one-time cold-object fetch still miss their retained budgets in `.plans/PR_REVIEW_PERF_PLAN.md`. +Progress note (2026-07-18): the specialized large-PR work now has real registered-PR measurements, stable review-thread identity, bounded local metadata reuse, active-patch priority, and passing first-patch and warm review-thread browser budgets. The workstream is complete for now. Production tree visibility, the one-time cold-object fetch, and uncached review-thread latency still miss their retained budgets and remain explicit future follow-ups in `.plans/PR_REVIEW_PERF_PLAN.md`; those misses have not been reclassified as passes. -Sequencing correction (2026-07-17): retain the completed Phase A foundation and PR #143, but do not advance into Phase B yet. Phase A was selected while the specialized performance plan still had partial acceptance; that ordering change was not an implicit deferral of the remaining measured misses. Resume the real registered-PR performance work first, beginning with review-thread latency, then reconcile tree visibility and the cold-fetch decision. Phase B remains paused until those items pass or are explicitly deferred with recorded rationale. +Historical sequencing correction (2026-07-17): retain the completed Phase A foundation and PR #143, but do not advance into Phase B yet. Phase A was selected while the specialized performance plan still had partial acceptance; that ordering change was not an implicit deferral of the remaining measured misses. Resume the real registered-PR performance work first, beginning with review-thread latency, then reconcile tree visibility and the cold-fetch decision. This pause was lifted on 2026-07-18 after the remaining misses were explicitly deferred with recorded rationale; the original correction remains here for audit history. Contract note (2026-07-18): `shared/review-source.ts` now defines the versioned source, revision, repository, capability, ordered-file, and explicit patch-state vocabulary used by every current web diff surface. GitHub PRs use head SHA identity; prepared and Kilo/repo worktree views receive content-addressed changed-path fingerprints from metadata reads; skill patches and historical repo-edit events use retained content hashes. Missing identities remain explicitly unavailable rather than falling back to timestamps. The current viewers expose source/revision metadata on their mounted roots, ready for the Phase A registration and navigation event layer. On a synthetic 305-file changed worktree, metadata plus revision identity measured 335.5 ms median versus 243.0 ms for metadata alone (92.5 ms added), within the 500 ms warm-tree budget. @@ -14,7 +14,7 @@ Fixture note (2026-07-18): `npm run bench:review-fixtures` now builds determinis Related plans: -- `.plans/PR_REVIEW_PERF_PLAN.md` — active large-PR data-path and performance workstream +- `.plans/PR_REVIEW_PERF_PLAN.md` — complete-for-now large-PR data-path and performance workstream with explicit deferred misses - `.plans/OTHER_PEOPLE_PR_REVIEW.md` — current human PR review workflow - `.plans/archived/DIFF_UI_PLAN.md` — landed Pierre diff/tree adoption - `.plans/archived/DIFF_REVIEW.md` — earlier diff review research and interaction planning @@ -545,7 +545,7 @@ before relaxing a gate. ### Phase B — Guided review -1. Add review-map decorations and pure cross-file cursors. +1. **Completed —** Add review-map decorations and pure cross-file cursors. 2. Add visible navigation controls, scoped shortcuts, and help. 3. Add typed Neon finding application and inline rendering with provenance. 4. Add explicit promote-to-draft and promote-to-revision flows. @@ -637,8 +637,9 @@ before relaxing a gate. - Add a changeset for each user-facing implementation phase. - Record deviations or deferrals in `.plans/DEVIATIONS.md` when implementation changes this priority order, trust boundary, performance gate, or surface coverage. -- Keep `.plans/PR_REVIEW_PERF_PLAN.md` as the specialized performance implementation plan until that - work lands; then archive it and preserve this document as the broader diff product roadmap. +- Retain `.plans/PR_REVIEW_PERF_PLAN.md` as the specialized performance implementation record while + its measured tree, cold-fetch, and uncached-thread misses remain deferred; archive it only after + those follow-ups are reconciled, and preserve this document as the broader diff product roadmap. ## Definition of Done diff --git a/.plans/PR_REVIEW_PERF_PLAN.md b/.plans/PR_REVIEW_PERF_PLAN.md index cddef3b2..a15164ce 100644 --- a/.plans/PR_REVIEW_PERF_PLAN.md +++ b/.plans/PR_REVIEW_PERF_PLAN.md @@ -1,8 +1,24 @@ # PR Review / File Tree Performance Plan -Status: phases 1–5 implemented; real-PR verification, request-path remediation, active-patch prioritization, and warm review-thread remediation complete; tree and cold-fetch decisions remain +Status: complete for now; phases 1–5, real-PR verification, request-path remediation, active-patch prioritization, and warm review-thread remediation are complete; the missed tree, cold-object-fetch, and cold-thread budgets are explicitly deferred below Prior art: `.plans/archived/DIFF_UI_PLAN.md`, `.plans/archived/DIFF_REVIEW.md` +## 2026-07-18 completion-for-now decision + +This specialized workstream is complete for now, not complete against every +retained budget. The production tree median remains 642 ms against the <500 ms +target. The one-time cold local object fetch remains 4,978 ms against the +<3,000 ms target. The uncached lean review-thread surface remained 1,511 ms, +and its initial GitHub-backed read remained 655 ms, outside the warm UI path +that reached a passing 459 ms median through bounded 15-second reuse. These +misses are deferred future performance work; none is reclassified as passing. + +The implemented data path, immutable measurements, and completed remediation +items remain active foundations for the broader diff plan. Phase B can proceed +because the remaining misses are now explicit, measured deferrals rather than +unreconciled acceptance gaps. Future work should remeasure the same immutable +target before changing implementation or relaxing a retained budget. + ## 2026-07-18 shared Phase A fixture baseline The broader diff-improvements Phase A now has deterministic 8-file, 90-file, @@ -351,16 +367,26 @@ resources. The cold first read remains explicitly outside the warm-cache pass. now uses an 84.7% smaller query response plus a bounded 15-second cache with mutation invalidation and race protection. Production thread visibility is 459 ms median, while the full-fidelity Flue action is unchanged. -5. **Discuss later — revisit cold fetch.** The 4.98-second object fetch misses the target, +5. **Deferred — revisit cold fetch.** The 4.98-second object fetch misses the target, but it is a one-time revision cost. Separate network fetch time from local metadata time before changing refspecs or the `<3s` budget. - -Acceptance is partial on the same real target: duplicate thread requests and +6. **Deferred — revisit production tree visibility.** The final 642 ms median + remains 142 ms over the `<500 ms` budget even though the warm backend + diagnostic passes. Profile the production boot/query/render boundary before + changing Pierre or the tree budget. +7. **Deferred — revisit uncached review-thread latency.** Bounded warm reuse + brings the median to 459 ms, but the uncached lean surface remained 1,511 ms + and the initial GitHub-backed read remained 655 ms. Preserve mutation + invalidation and cancellation while evaluating any more durable reuse or + GitHub query-path change. + +Acceptance remains partial on the same real target: duplicate thread requests and settlement-driven abandoned patch reads are eliminated, the first-patch browser budget now passes, warm thread visibility passes on the median, backend targets pass, and fallback code is unchanged. The tree and one-time cold-object budgets -still miss and remain separate follow-ups; a cold GitHub thread read also -remains slower than the warm UI budget. Raw baseline, remediation, and +still miss and are explicitly deferred; a cold GitHub thread read also remains +slower than the warm UI budget and is explicitly deferred. The workstream is +complete for now on that basis. Raw baseline, remediation, and active-priority results are gitignored at `benchmarks/results/pr-12204-real-local.json` and diff --git a/shared/review-navigation.ts b/shared/review-navigation.ts new file mode 100644 index 00000000..bd987531 --- /dev/null +++ b/shared/review-navigation.ts @@ -0,0 +1,505 @@ +export type ReviewFindingSeverity = 'critical' | 'major' | 'minor' | 'nit'; + +export type ReviewNavigationFile = { + path: string; + previousPath?: string | null; +}; + +type ReviewNavigationItemBase = { + id: string; + path: string; + summary?: string | null; + stale?: boolean; +}; + +export type ReviewHunkNavigationItem = ReviewNavigationItemBase & { + kind: 'hunk'; + oldStart?: number | null; + newStart?: number | null; +}; + +export type ReviewThreadNavigationItem = ReviewNavigationItemBase & { + kind: 'review-thread'; + line?: number | null; + resolved?: boolean; +}; + +export type ReviewDraftNavigationItem = ReviewNavigationItemBase & { + kind: 'local-draft'; + line?: number | null; +}; + +export type ReviewFindingNavigationItem = ReviewNavigationItemBase & { + kind: 'finding'; + line?: number | null; + severity?: ReviewFindingSeverity | null; +}; + +export type ReviewNavigationItem = + | ReviewHunkNavigationItem + | ReviewThreadNavigationItem + | ReviewDraftNavigationItem + | ReviewFindingNavigationItem; + +export type ReviewNavigationInput = { + files: readonly ReviewNavigationFile[]; + items?: readonly ReviewNavigationItem[]; + guidedOrder?: readonly string[]; +}; + +export type ReviewNavigationTargetKind = 'file' | ReviewNavigationItem['kind']; + +export type ReviewCursorKind = ReviewNavigationTargetKind | 'attention'; + +export type ReviewNavigationTarget = { + key: string; + kind: ReviewNavigationTargetKind; + id: string; + path: string; + requestedPath: string; + previousPath: string | null; + fileIndex: number; + position: number; + summary: string | null; + severity: ReviewFindingSeverity | null; + stale: boolean; + missing: boolean; +}; + +export type ReviewAttentionTarget = Omit< + ReviewNavigationTarget, + 'key' | 'kind' +> & { + key: string; + kind: 'attention'; + attentionKind: 'review-thread' | 'local-draft' | 'finding'; + targetKey: string; +}; + +export type ReviewCursorTarget = ReviewNavigationTarget | ReviewAttentionTarget; + +export type ReviewNavigationModel = { + canonicalFilePaths: readonly string[]; + guidedFilePaths: readonly string[]; + targets: readonly ReviewNavigationTarget[]; + attentionTargets: readonly ReviewAttentionTarget[]; + unavailableTargets: readonly ReviewNavigationTarget[]; +}; + +export type ReviewNavigationFilter = { + includeMissing?: boolean; + includeStale?: boolean; + kinds?: readonly ReviewCursorKind[]; + paths?: readonly string[]; + query?: string | null; +}; + +export type ReviewCursorOrder = 'canonical' | 'guided'; +export type ReviewCursorDirection = 'previous' | 'next'; + +export type ReviewCursorResult = { + target: ReviewCursorTarget | null; + index: number; + total: number; + resolution: 'empty' | 'initial' | 'exact' | 'nearest'; + boundary: 'start' | 'end' | null; +}; + +const targetKindOrder: Record = { + file: 0, + hunk: 1, + 'review-thread': 2, + 'local-draft': 3, + finding: 4, +}; + +export function createReviewNavigationModel( + input: ReviewNavigationInput, +): ReviewNavigationModel { + const files = uniqueFiles(input.files); + const canonicalFilePaths = files.map((file) => file.path); + const fileIndexByPath = new Map( + canonicalFilePaths.map((path, index) => [path, index]), + ); + const previousPathByPath = new Map( + files.map((file) => [file.path, file.previousPath ?? null]), + ); + const aliasToPath = unambiguousPreviousPathAliases(files); + const guidedFilePaths = normalizeGuidedOrder( + input.guidedOrder ?? [], + canonicalFilePaths, + aliasToPath, + ); + const seenKeys = new Set(); + const targets: ReviewNavigationTarget[] = canonicalFilePaths.map( + (path, fileIndex) => ({ + fileIndex, + id: path, + key: targetKey('file', path), + kind: 'file', + missing: false, + path, + position: 0, + previousPath: previousPathByPath.get(path) ?? null, + requestedPath: path, + severity: null, + stale: false, + summary: null, + }), + ); + for (const target of targets) seenKeys.add(target.key); + + const unavailableTargets: ReviewNavigationTarget[] = []; + for (const [inputIndex, item] of (input.items ?? []).entries()) { + if (item.kind === 'review-thread' && item.resolved) continue; + const key = targetKey(item.kind, item.id); + if (seenKeys.has(key)) continue; + seenKeys.add(key); + const resolvedPath = resolvePath(item.path, fileIndexByPath, aliasToPath); + const missing = resolvedPath === null; + const path = resolvedPath ?? item.path; + const target: ReviewNavigationTarget = { + fileIndex: resolvedPath + ? (fileIndexByPath.get(resolvedPath) ?? canonicalFilePaths.length) + : canonicalFilePaths.length, + id: item.id, + key, + kind: item.kind, + missing, + path, + position: itemPosition(item, inputIndex), + previousPath: resolvedPath + ? (previousPathByPath.get(resolvedPath) ?? null) + : null, + requestedPath: item.path, + severity: item.kind === 'finding' ? (item.severity ?? null) : null, + stale: Boolean(item.stale || missing), + summary: item.summary?.trim() || null, + }; + targets.push(target); + if (missing) unavailableTargets.push(target); + } + + targets.sort(targetComparator(fileIndexByPath)); + const attentionTargets = targets + .filter( + ( + target, + ): target is ReviewNavigationTarget & { + kind: 'review-thread' | 'local-draft' | 'finding'; + } => + target.kind === 'review-thread' || + target.kind === 'local-draft' || + target.kind === 'finding', + ) + .map((target) => ({ + ...target, + attentionKind: target.kind, + key: `attention:${target.key}`, + kind: 'attention' as const, + targetKey: target.key, + })); + + return { + attentionTargets, + canonicalFilePaths, + guidedFilePaths, + targets, + unavailableTargets, + }; +} + +type ReviewCursorTargetOptions = { + filter?: ReviewNavigationFilter; + order?: ReviewCursorOrder; +}; + +export function reviewCursorTargets( + model: ReviewNavigationModel, + kind: 'attention', + options?: ReviewCursorTargetOptions, +): ReviewAttentionTarget[]; +export function reviewCursorTargets( + model: ReviewNavigationModel, + kind: ReviewNavigationTargetKind, + options?: ReviewCursorTargetOptions, +): ReviewNavigationTarget[]; +export function reviewCursorTargets( + model: ReviewNavigationModel, + kind: ReviewCursorKind, + options: ReviewCursorTargetOptions = {}, +): ReviewCursorTarget[] { + const source = + kind === 'attention' + ? model.attentionTargets + : model.targets.filter((target) => target.kind === kind); + const order = options.order ?? 'canonical'; + const fileOrder = + order === 'guided' ? model.guidedFilePaths : model.canonicalFilePaths; + const fileIndexByPath = new Map( + fileOrder.map((path, index) => [path, index]), + ); + const filter = options.filter; + return source + .filter((target) => matchesFilter(target, kind, filter)) + .sort(targetComparator(fileIndexByPath)); +} + +export function moveReviewCursor( + targets: readonly ReviewCursorTarget[], + current: string | ReviewCursorTarget | null, + direction: ReviewCursorDirection, +): ReviewCursorResult { + if (targets.length === 0) return emptyCursorResult(); + if (current === null) { + const index = direction === 'next' ? 0 : targets.length - 1; + return cursorResult(targets, index, 'initial', null); + } + + const currentKey = typeof current === 'string' ? current : current.key; + const currentIndex = targets.findIndex((target) => target.key === currentKey); + if (currentIndex < 0) { + const nearestIndex = nearestTargetIndex( + targets, + typeof current === 'string' ? null : current, + ); + return cursorResult(targets, nearestIndex, 'nearest', null); + } + + const nextIndex = currentIndex + (direction === 'next' ? 1 : -1); + if (nextIndex < 0) { + return cursorResult(targets, currentIndex, 'exact', 'start'); + } + if (nextIndex >= targets.length) { + return cursorResult(targets, currentIndex, 'exact', 'end'); + } + return cursorResult(targets, nextIndex, 'exact', null); +} + +export function reconcileReviewCursor( + previousTargets: readonly ReviewCursorTarget[], + nextTargets: readonly ReviewCursorTarget[], + currentKey: string | null, +): ReviewCursorResult { + if (nextTargets.length === 0) return emptyCursorResult(); + const exactIndex = currentKey + ? nextTargets.findIndex((target) => target.key === currentKey) + : -1; + if (exactIndex >= 0) { + return cursorResult(nextTargets, exactIndex, 'exact', null); + } + const previousTarget = currentKey + ? (previousTargets.find((target) => target.key === currentKey) ?? null) + : null; + const nearestIndex = nearestTargetIndex(nextTargets, previousTarget); + return cursorResult( + nextTargets, + nearestIndex, + previousTarget ? 'nearest' : 'initial', + null, + ); +} + +function uniqueFiles(files: readonly ReviewNavigationFile[]) { + const seen = new Set(); + return files.filter((file) => { + const path = file.path.trim(); + if (!path || seen.has(path)) return false; + seen.add(path); + return true; + }); +} + +function unambiguousPreviousPathAliases( + files: readonly ReviewNavigationFile[], +) { + const aliases = new Map(); + for (const file of files) { + const previousPath = file.previousPath?.trim(); + if (!previousPath || previousPath === file.path) continue; + const existing = aliases.get(previousPath); + aliases.set( + previousPath, + existing === undefined || existing === file.path ? file.path : null, + ); + } + return new Map( + [...aliases].filter( + (entry): entry is [string, string] => entry[1] !== null, + ), + ); +} + +function normalizeGuidedOrder( + requestedOrder: readonly string[], + canonicalOrder: readonly string[], + aliases: ReadonlyMap, +) { + const canonicalPaths = new Set(canonicalOrder); + const seen = new Set(); + const guided: string[] = []; + for (const requestedPath of requestedOrder) { + const path = canonicalPaths.has(requestedPath) + ? requestedPath + : aliases.get(requestedPath); + if (!path || seen.has(path)) continue; + seen.add(path); + guided.push(path); + } + for (const path of canonicalOrder) { + if (!seen.has(path)) guided.push(path); + } + return guided; +} + +function resolvePath( + path: string, + fileIndexByPath: ReadonlyMap, + aliases: ReadonlyMap, +) { + if (fileIndexByPath.has(path)) return path; + return aliases.get(path) ?? null; +} + +function itemPosition(item: ReviewNavigationItem, inputIndex: number) { + const line = + item.kind === 'hunk' + ? (positiveNumber(item.newStart) ?? positiveNumber(item.oldStart)) + : positiveNumber(item.line); + return line ?? 1_000_000_000 + inputIndex; +} + +function positiveNumber(value: number | null | undefined) { + return typeof value === 'number' && value >= 0 ? value : null; +} + +function targetKey(kind: ReviewNavigationTargetKind, id: string) { + return `${kind}:${id}`; +} + +function targetComparator(fileIndexByPath: ReadonlyMap) { + return (left: ReviewCursorTarget, right: ReviewCursorTarget) => { + const leftFileIndex = + fileIndexByPath.get(left.path) ?? Number.MAX_SAFE_INTEGER; + const rightFileIndex = + fileIndexByPath.get(right.path) ?? Number.MAX_SAFE_INTEGER; + return ( + leftFileIndex - rightFileIndex || + left.position - right.position || + cursorKindOrder(left) - cursorKindOrder(right) || + left.key.localeCompare(right.key) + ); + }; +} + +function cursorKindOrder(target: ReviewCursorTarget) { + return target.kind === 'attention' + ? targetKindOrder[target.attentionKind] + : targetKindOrder[target.kind]; +} + +function matchesFilter( + target: ReviewCursorTarget, + kind: ReviewCursorKind, + filter: ReviewNavigationFilter | undefined, +) { + if (!filter) return !target.missing; + if (!filter.includeMissing && target.missing) return false; + if (filter.includeStale === false && target.stale) return false; + const targetKind = + target.kind === 'attention' ? target.attentionKind : target.kind; + if ( + filter.kinds && + !filter.kinds.includes(kind) && + !filter.kinds.includes(targetKind) + ) { + return false; + } + if ( + filter.paths && + !filter.paths.some((path) => targetMatchesPath(target, path)) + ) { + return false; + } + const query = filter.query?.trim().toLocaleLowerCase(); + if (!query) return true; + return [ + target.id, + target.path, + target.previousPath, + target.requestedPath, + target.summary, + target.severity, + target.kind === 'attention' ? target.attentionKind : target.kind, + ].some((value) => value?.toLocaleLowerCase().includes(query)); +} + +function targetMatchesPath(target: ReviewCursorTarget, path: string) { + return ( + target.path === path || + target.previousPath === path || + target.requestedPath === path + ); +} + +function nearestTargetIndex( + targets: readonly ReviewCursorTarget[], + anchor: ReviewCursorTarget | null, +) { + if (!anchor) return 0; + let bestIndex = 0; + let bestScore: readonly number[] | null = null; + for (const [index, target] of targets.entries()) { + const sameFile = targetMatchesPath(target, anchor.path) ? 0 : 1; + const fileDistance = Math.abs(target.fileIndex - anchor.fileIndex); + const preferFollowingFile = target.fileIndex >= anchor.fileIndex ? 0 : 1; + const positionDistance = Math.abs(target.position - anchor.position); + const preferFollowingPosition = target.position >= anchor.position ? 0 : 1; + const score = [ + sameFile, + fileDistance, + preferFollowingFile, + positionDistance, + preferFollowingPosition, + index, + ] as const; + if (!bestScore || compareScore(score, bestScore) < 0) { + bestIndex = index; + bestScore = score; + } + } + return bestIndex; +} + +function compareScore(left: readonly number[], right: readonly number[]) { + for (let index = 0; index < left.length; index += 1) { + const difference = (left[index] ?? 0) - (right[index] ?? 0); + if (difference !== 0) return difference; + } + return 0; +} + +function emptyCursorResult(): ReviewCursorResult { + return { + boundary: null, + index: -1, + resolution: 'empty', + target: null, + total: 0, + }; +} + +function cursorResult( + targets: readonly ReviewCursorTarget[], + index: number, + resolution: Exclude, + boundary: ReviewCursorResult['boundary'], +): ReviewCursorResult { + return { + boundary, + index, + resolution, + target: targets[index] ?? null, + total: targets.length, + }; +} diff --git a/src/review-navigation.test.ts b/src/review-navigation.test.ts new file mode 100644 index 00000000..9fc927f5 --- /dev/null +++ b/src/review-navigation.test.ts @@ -0,0 +1,347 @@ +import { describe, expect, it } from 'vitest'; +import { + createReviewNavigationModel, + moveReviewCursor, + reconcileReviewCursor, + reviewCursorTargets, + type ReviewNavigationInput, +} from '../shared/review-navigation'; + +describe('review navigation model', () => { + it('keeps canonical order while resolving a complete guided projection', () => { + const model = createReviewNavigationModel({ + files: [{ path: 'src/a.ts' }, { path: 'src/b.ts' }, { path: 'src/c.ts' }], + guidedOrder: ['src/c.ts', 'src/c.ts', 'missing.ts'], + }); + + expect(model.canonicalFilePaths).toEqual([ + 'src/a.ts', + 'src/b.ts', + 'src/c.ts', + ]); + expect(model.guidedFilePaths).toEqual(['src/c.ts', 'src/a.ts', 'src/b.ts']); + expect( + reviewCursorTargets(model, 'file', { order: 'guided' }).map( + (target) => target.path, + ), + ).toEqual(['src/c.ts', 'src/a.ts', 'src/b.ts']); + }); + + it('normalizes file, hunk, thread, draft, finding, and attention targets', () => { + const model = createReviewNavigationModel(fixture()); + + expect(reviewCursorTargets(model, 'file')).toHaveLength(3); + expect( + reviewCursorTargets(model, 'hunk').map((target) => target.id), + ).toEqual(['hunk-a-1', 'hunk-a-2']); + expect( + reviewCursorTargets(model, 'review-thread').map((target) => target.id), + ).toEqual(['thread-a']); + expect( + reviewCursorTargets(model, 'local-draft').map((target) => target.id), + ).toEqual(['draft-a', 'draft-b']); + expect( + reviewCursorTargets(model, 'finding').map((target) => target.id), + ).toEqual(['finding-a']); + expect( + reviewCursorTargets(model, 'attention').map((target) => [ + target.attentionKind, + target.id, + ]), + ).toEqual([ + ['review-thread', 'thread-a'], + ['local-draft', 'draft-a'], + ['finding', 'finding-a'], + ['local-draft', 'draft-b'], + ]); + }); + + it('deduplicates files and target identities deterministically', () => { + const model = createReviewNavigationModel({ + files: [ + { path: 'src/a.ts' }, + { path: 'src/a.ts', previousPath: 'src/old-a.ts' }, + ], + items: [ + { kind: 'local-draft', id: 'draft-1', path: 'src/a.ts', line: 8 }, + { kind: 'local-draft', id: 'draft-1', path: 'src/a.ts', line: 2 }, + ], + }); + + expect(model.canonicalFilePaths).toEqual(['src/a.ts']); + expect(reviewCursorTargets(model, 'local-draft')).toMatchObject([ + { id: 'draft-1', position: 8 }, + ]); + }); + + it('resolves targets on previous paths to renamed files', () => { + const model = createReviewNavigationModel({ + files: [{ path: 'src/new.ts', previousPath: 'src/old.ts' }], + guidedOrder: ['src/old.ts'], + items: [ + { + kind: 'review-thread', + id: 'thread-old', + path: 'src/old.ts', + line: 4, + }, + ], + }); + const target = reviewCursorTargets(model, 'review-thread')[0]; + + expect(model.guidedFilePaths).toEqual(['src/new.ts']); + expect(target).toMatchObject({ + missing: false, + path: 'src/new.ts', + previousPath: 'src/old.ts', + requestedPath: 'src/old.ts', + stale: false, + }); + }); + + it('reconciles a renamed file cursor through its previous path', () => { + const before = createReviewNavigationModel({ + files: [{ path: 'src/old.ts' }], + }); + const after = createReviewNavigationModel({ + files: [{ path: 'src/new.ts', previousPath: 'src/old.ts' }], + }); + + expect( + reconcileReviewCursor( + reviewCursorTargets(before, 'file'), + reviewCursorTargets(after, 'file'), + 'file:src/old.ts', + ), + ).toMatchObject({ + resolution: 'nearest', + target: { path: 'src/new.ts', previousPath: 'src/old.ts' }, + }); + }); + + it('retains removed-file targets as unavailable without navigating to them', () => { + const model = createReviewNavigationModel({ + files: [{ path: 'src/live.ts' }], + items: [ + { + kind: 'finding', + id: 'removed-finding', + path: 'src/removed.ts', + severity: 'major', + }, + ], + }); + + expect(reviewCursorTargets(model, 'finding')).toEqual([]); + expect(model.unavailableTargets).toMatchObject([ + { + id: 'removed-finding', + missing: true, + stale: true, + }, + ]); + expect( + reviewCursorTargets(model, 'finding', { + filter: { includeMissing: true }, + }), + ).toHaveLength(1); + }); + + it('filters by current path, previous path, summary, kind, and stale state', () => { + const model = createReviewNavigationModel(fixture()); + + expect( + reviewCursorTargets(model, 'finding', { + filter: { query: 'unsafe fallback' }, + }).map((target) => target.id), + ).toEqual(['finding-a']); + expect( + reviewCursorTargets(model, 'attention', { + filter: { query: 'src/old-a.ts' }, + }).map((target) => target.id), + ).toEqual(['thread-a', 'draft-a', 'finding-a']); + expect( + reviewCursorTargets(model, 'local-draft', { + filter: { includeStale: false }, + }).map((target) => target.id), + ).toEqual(['draft-a']); + expect( + reviewCursorTargets(model, 'local-draft').find( + (target) => target.id === 'draft-b', + ), + ).toMatchObject({ stale: true }); + expect( + reviewCursorTargets(model, 'attention', { + filter: { kinds: ['finding'] }, + }).map((target) => target.id), + ).toEqual(['finding-a']); + }); + + it('moves deterministically at empty, initial, previous, next, and boundary states', () => { + const targets = reviewCursorTargets( + createReviewNavigationModel(fixture()), + 'hunk', + ); + + expect(moveReviewCursor([], null, 'next')).toMatchObject({ + boundary: null, + resolution: 'empty', + target: null, + }); + expect(moveReviewCursor(targets, null, 'next')).toMatchObject({ + index: 0, + resolution: 'initial', + target: { id: 'hunk-a-1' }, + }); + expect(moveReviewCursor(targets, null, 'previous')).toMatchObject({ + index: 1, + target: { id: 'hunk-a-2' }, + }); + expect(moveReviewCursor(targets, targets[0] ?? null, 'next')).toMatchObject( + { + boundary: null, + index: 1, + target: { id: 'hunk-a-2' }, + }, + ); + expect( + moveReviewCursor(targets, targets[0] ?? null, 'previous'), + ).toMatchObject({ + boundary: 'start', + index: 0, + }); + expect(moveReviewCursor(targets, targets[1] ?? null, 'next')).toMatchObject( + { + boundary: 'end', + index: 1, + }, + ); + }); + + it('uses the nearest following target when a stale current target is missing', () => { + const model = createReviewNavigationModel({ + files: [{ path: 'src/a.ts' }, { path: 'src/b.ts' }], + items: [ + { kind: 'local-draft', id: 'draft-a', path: 'src/a.ts', line: 10 }, + { kind: 'local-draft', id: 'draft-b', path: 'src/a.ts', line: 20 }, + { kind: 'local-draft', id: 'draft-c', path: 'src/b.ts', line: 1 }, + ], + }); + const targets = reviewCursorTargets(model, 'local-draft'); + const staleAnchor = { + ...targets[0]!, + key: 'local-draft:removed', + position: 15, + }; + + expect(moveReviewCursor(targets, staleAnchor, 'next')).toMatchObject({ + resolution: 'nearest', + target: { id: 'draft-b' }, + }); + }); + + it('preserves exact targets across filtering and falls back to the nearest remaining target', () => { + const model = createReviewNavigationModel(fixture()); + const allTargets = reviewCursorTargets(model, 'attention'); + const filteredTargets = reviewCursorTargets(model, 'attention', { + filter: { paths: ['src/b.ts'] }, + }); + + expect( + reconcileReviewCursor( + allTargets, + allTargets, + 'attention:local-draft:draft-a', + ), + ).toMatchObject({ + resolution: 'exact', + target: { id: 'draft-a' }, + }); + expect( + reconcileReviewCursor( + allTargets, + filteredTargets, + 'attention:finding:finding-a', + ), + ).toMatchObject({ + resolution: 'nearest', + target: { id: 'draft-b', path: 'src/b.ts' }, + }); + }); + + it('falls forward to a neighboring file when the active file is removed', () => { + const before = createReviewNavigationModel({ + files: [{ path: 'src/a.ts' }, { path: 'src/b.ts' }, { path: 'src/c.ts' }], + }); + const after = createReviewNavigationModel({ + files: [{ path: 'src/a.ts' }, { path: 'src/c.ts' }], + }); + const previousTargets = reviewCursorTargets(before, 'file'); + const nextTargets = reviewCursorTargets(after, 'file'); + + expect( + reconcileReviewCursor(previousTargets, nextTargets, 'file:src/b.ts'), + ).toMatchObject({ + resolution: 'nearest', + target: { path: 'src/c.ts' }, + }); + }); +}); + +function fixture(): ReviewNavigationInput { + return { + files: [ + { path: 'src/a.ts', previousPath: 'src/old-a.ts' }, + { path: 'src/b.ts' }, + { path: 'src/deleted.ts' }, + ], + items: [ + { + kind: 'hunk', + id: 'hunk-a-2', + path: 'src/a.ts', + newStart: 20, + }, + { + kind: 'hunk', + id: 'hunk-a-1', + path: 'src/a.ts', + newStart: 4, + }, + { + kind: 'review-thread', + id: 'thread-a', + path: 'src/old-a.ts', + line: 5, + }, + { + kind: 'review-thread', + id: 'resolved-thread', + path: 'src/a.ts', + line: 6, + resolved: true, + }, + { + kind: 'local-draft', + id: 'draft-a', + path: 'src/a.ts', + line: 7, + }, + { + kind: 'finding', + id: 'finding-a', + path: 'src/a.ts', + line: 8, + severity: 'critical', + summary: 'Unsafe fallback can erase data', + }, + { + kind: 'local-draft', + id: 'draft-b', + path: 'src/b.ts', + line: 3, + stale: true, + }, + ], + }; +} diff --git a/web/src/features/diff-viewer/FileTreePane.test.tsx b/web/src/features/diff-viewer/FileTreePane.test.tsx new file mode 100644 index 00000000..0479166a --- /dev/null +++ b/web/src/features/diff-viewer/FileTreePane.test.tsx @@ -0,0 +1,144 @@ +// @vitest-environment jsdom + +import { act } from 'react'; +import { createRoot } from 'react-dom/client'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { + FileTreePane, + fileReviewMapDecoration, + fileReviewMapStatusLabel, +} from './FileTreePane'; +import type { DiffFilePatch, FileReviewMapEntry } from './types'; + +describe('FileTreePane review map', () => { + let container: HTMLDivElement; + let root: ReturnType; + + beforeEach(() => { + ( + globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean } + ).IS_REACT_ACT_ENVIRONMENT = true; + container = document.createElement('div'); + document.body.append(container); + root = createRoot(container); + }); + + afterEach(() => { + act(() => root.unmount()); + container.remove(); + vi.restoreAllMocks(); + }); + + it('renders compact text and a full accessible status label', () => { + const entry = reviewEntry({ + draftCount: 3, + findingCount: 2, + highestFindingSeverity: 'critical', + staleDraftCount: 1, + unresolvedThreadCount: 4, + }); + + expect(fileReviewMapDecoration(entry)).toBe('T4 D3 S1 N2 critical'); + expect(fileReviewMapStatusLabel(entry, true)).toBe( + 'src/a.ts: 4 unresolved review threads, 3 local drafts, 1 stale draft, 2 Neon findings, highest severity critical.', + ); + }); + + it('updates review counts without replacing Pierre selection state', async () => { + const onSelectPath = vi.fn<(path: string) => void>(); + const files = reviewFiles(); + const initialMap = new Map([ + ['src/a.ts', reviewEntry({ unresolvedThreadCount: 1 })], + ]); + + await act(async () => { + root.render( + , + ); + }); + + expect(selectedPaths()).toEqual(['src/a.ts']); + expect(decorationText('src/a.ts')).toBe('T1'); + expect(container.textContent).toContain( + 'src/a.ts: 1 unresolved review thread.', + ); + + const updatedMap = new Map([ + ['src/a.ts', reviewEntry({ draftCount: 2, unresolvedThreadCount: 1 })], + ]); + await act(async () => { + root.render( + , + ); + }); + + expect(selectedPaths()).toEqual(['src/a.ts']); + expect(decorationText('src/a.ts')).toBe('T1 D2'); + expect(container.textContent).toContain( + 'src/a.ts: 1 unresolved review thread, 2 local drafts.', + ); + + await act(async () => { + treeItem('src/b.ts')?.click(); + }); + expect(onSelectPath).toHaveBeenCalledWith('src/b.ts'); + }); + + function treeHost() { + return container.querySelector('file-tree-container'); + } + + function treeItem(path: string) { + return treeHost()?.shadowRoot?.querySelector( + `[data-item-path="${path}"]`, + ); + } + + function selectedPaths() { + return [ + ...(treeHost()?.shadowRoot?.querySelectorAll('[data-item-selected]') ?? + []), + ] + .map((item) => item.getAttribute('data-item-path')) + .filter(Boolean); + } + + function decorationText(path: string) { + return treeItem(path) + ?.querySelector('[data-item-section="decoration"]') + ?.textContent?.trim(); + } +}); + +function reviewFiles(): DiffFilePatch[] { + return ['src/a.ts', 'src/b.ts'].map((path) => ({ + additions: 1, + deletions: 1, + path, + status: 'modified', + })); +} + +function reviewEntry( + overrides: Partial> = {}, +): FileReviewMapEntry { + return { + draftCount: 0, + findingCount: 0, + highestFindingSeverity: null, + path: 'src/a.ts', + staleDraftCount: 0, + unresolvedThreadCount: 0, + ...overrides, + }; +} diff --git a/web/src/features/diff-viewer/FileTreePane.tsx b/web/src/features/diff-viewer/FileTreePane.tsx index 9a955b24..e273e796 100644 --- a/web/src/features/diff-viewer/FileTreePane.tsx +++ b/web/src/features/diff-viewer/FileTreePane.tsx @@ -1,5 +1,5 @@ import { FileTree, useFileTree } from '@pierre/trees/react'; -import { useEffect, useMemo } from 'react'; +import { useEffect, useMemo, useRef } from 'react'; import { prepareFileTreeInput, type FileTreeInitialExpansion, @@ -7,7 +7,7 @@ import { type GitStatusEntry, } from '@pierre/trees'; import { diffStatsLabel } from './helpers'; -import type { DiffFilePatch } from './types'; +import type { DiffFilePatch, FileReviewMapEntry } from './types'; const expandedTreeFileLimit = 120; @@ -15,12 +15,14 @@ type FileTreePaneProps = { files: DiffFilePatch[]; selectedPath: string | null; onSelectPath: (path: string) => void; + reviewMapByPath?: ReadonlyMap; }; export function FileTreePane({ files, selectedPath, onSelectPath, + reviewMapByPath, }: FileTreePaneProps) { const paths = useMemo(() => files.map((file) => file.path), [files]); const gitStatus = useMemo( @@ -46,6 +48,7 @@ export function FileTreePane({ onSelectPath={onSelectPath} paths={paths} preparedInput={preparedInput} + reviewMapByPath={reviewMapByPath} selectedPath={selectedPath} /> ); @@ -58,6 +61,7 @@ function FileTreePaneModel({ onSelectPath, paths, preparedInput, + reviewMapByPath, selectedPath, }: FileTreePaneProps & { gitStatus: GitStatusEntry[]; @@ -65,6 +69,8 @@ function FileTreePaneModel({ paths: string[]; preparedInput: FileTreePreparedInput; }) { + const reviewMapRef = useRef(reviewMapByPath); + reviewMapRef.current = reviewMapByPath; const filePathSet = useMemo(() => new Set(paths), [paths]); const { model } = useFileTree({ flattenEmptyDirectories: true, @@ -79,6 +85,16 @@ function FileTreePaneModel({ if (nextPath) onSelectPath(nextPath); }, preparedInput, + renderRowDecoration({ item }) { + if (item.kind !== 'file') return null; + const entry = reviewMapRef.current?.get(item.path); + return entry && fileReviewMapHasStatus(entry) + ? { + text: fileReviewMapDecoration(entry), + title: fileReviewMapStatusLabel(entry), + } + : null; + }, search: true, searchBlurBehavior: 'retain', unsafeCSS: treeUnsafeCss, @@ -98,33 +114,111 @@ function FileTreePaneModel({ model.setGitStatus(gitStatus); }, [gitStatus, model]); + useEffect(() => { + model.setComposition(model.getComposition()); + }, [model, reviewMapByPath]); + + const selectedReviewStatus = selectedPath + ? reviewMapByPath?.get(selectedPath) + : null; + return (
} + aria-label={ + reviewMapByPath ? 'Changed files with review status' : 'Changed files' + } + header={} model={model} style={{ height: '100%' }} /> + {selectedReviewStatus ? ( +

+ {fileReviewMapStatusLabel(selectedReviewStatus, true)} +

+ ) : null}
); } -function TreeHeader({ files }: { files: DiffFilePatch[] }) { +function TreeHeader({ + files, + reviewMapByPath, +}: { + files: DiffFilePatch[]; + reviewMapByPath?: ReadonlyMap; +}) { const additions = files.reduce((sum, file) => sum + (file.additions ?? 0), 0); const deletions = files.reduce((sum, file) => sum + (file.deletions ?? 0), 0); const largeSet = files.length > expandedTreeFileLimit; + const hasReviewStatus = [...(reviewMapByPath?.values() ?? [])].some( + fileReviewMapHasStatus, + ); return ( -
- files - - +{additions} -{deletions} - {largeSet ? - large set : null} - +
+
+ files + + +{additions} -{deletions} + {largeSet ? - large set : null} + +
+ {hasReviewStatus ? ( +

+ review map · T threads · D drafts · S stale · N Neon +

+ ) : null}
); } +export function fileReviewMapHasStatus(entry: FileReviewMapEntry) { + return ( + entry.unresolvedThreadCount > 0 || + entry.draftCount > 0 || + entry.staleDraftCount > 0 || + entry.findingCount > 0 + ); +} + +export function fileReviewMapDecoration(entry: FileReviewMapEntry) { + return [ + entry.unresolvedThreadCount > 0 ? `T${entry.unresolvedThreadCount}` : null, + entry.draftCount > 0 ? `D${entry.draftCount}` : null, + entry.staleDraftCount > 0 ? `S${entry.staleDraftCount}` : null, + entry.findingCount > 0 ? `N${entry.findingCount}` : null, + entry.findingCount > 0 ? entry.highestFindingSeverity : null, + ] + .filter(Boolean) + .join(' '); +} + +export function fileReviewMapStatusLabel( + entry: FileReviewMapEntry, + includePath = false, +) { + const status = [ + countLabel(entry.unresolvedThreadCount, 'unresolved review thread'), + countLabel(entry.draftCount, 'local draft'), + countLabel(entry.staleDraftCount, 'stale draft'), + countLabel(entry.findingCount, 'Neon finding'), + entry.findingCount > 0 && entry.highestFindingSeverity + ? `highest severity ${entry.highestFindingSeverity}` + : null, + ] + .filter(Boolean) + .join(', '); + const label = status || 'no review items'; + return includePath ? `${entry.path}: ${label}.` : label; +} + +function countLabel(count: number, label: string) { + return count > 0 ? `${count} ${label}${count === 1 ? '' : 's'}` : null; +} + function gitStatusEntry(file: DiffFilePatch): GitStatusEntry | null { const status = file.status.toLowerCase(); if (status === 'a' || status === 'added') { @@ -174,6 +268,10 @@ const treeUnsafeCss = ` outline: 1px solid var(--trees-selected-bg-override); outline-offset: 0; } + [data-item-section='decoration'] { + color: var(--trees-fg-override); + font-variant-numeric: tabular-nums; + } `; export function fileTreeSummary(files: DiffFilePatch[]) { diff --git a/web/src/features/diff-viewer/MultiFileView.tsx b/web/src/features/diff-viewer/MultiFileView.tsx index 9ca4b144..256de460 100644 --- a/web/src/features/diff-viewer/MultiFileView.tsx +++ b/web/src/features/diff-viewer/MultiFileView.tsx @@ -15,6 +15,7 @@ import type { DiffFilePatch, DiffReviewAnnotation, DiffViewTone, + FileReviewMapEntry, } from './types'; import type { ReviewSourceSnapshot } from '../../../../shared/review-source'; import { reviewSourceDataAttributes } from './review-source'; @@ -39,6 +40,7 @@ type MultiFileViewProps = { inspector?: ReactNode; inspectorLabel?: string; source?: ReviewSourceSnapshot; + reviewMapByPath?: ReadonlyMap; }; export function MultiFileView({ @@ -58,6 +60,7 @@ export function MultiFileView({ inspector, inspectorLabel = 'Diff inspector', source, + reviewMapByPath, title, tone = 'primary', }: MultiFileViewProps) { @@ -116,6 +119,7 @@ export function MultiFileView({ diff --git a/web/src/features/diff-viewer/types.ts b/web/src/features/diff-viewer/types.ts index 758e1fa5..ac876505 100644 --- a/web/src/features/diff-viewer/types.ts +++ b/web/src/features/diff-viewer/types.ts @@ -1,8 +1,19 @@ import type { RepoDiffFile } from '../../api'; +import type { ReviewFindingSeverity } from '../../../../shared/review-navigation'; export type DiffFilePatch = Omit & { patch?: string | null; message?: string | null; + previousPath?: string | null; +}; + +export type FileReviewMapEntry = { + path: string; + unresolvedThreadCount: number; + draftCount: number; + staleDraftCount: number; + findingCount: number; + highestFindingSeverity: ReviewFindingSeverity | null; }; export type DiffViewTone = 'primary' | 'violet' | 'accent'; diff --git a/web/src/features/pr-review/GitHubPrReview.test.ts b/web/src/features/pr-review/GitHubPrReview.test.ts index e1d2a99f..31de4bb7 100644 --- a/web/src/features/pr-review/GitHubPrReview.test.ts +++ b/web/src/features/pr-review/GitHubPrReview.test.ts @@ -19,6 +19,7 @@ import { import { backgroundReviewPatchPaths, draftCommentIdsWithUnknownPatch, + prReviewMapByPath, reviewPatchQuerySettled, } from './review-view-model'; import { @@ -352,6 +353,63 @@ describe('GitHubPrReview helpers', () => { ).toEqual(new Set(['deferred'])); }); + it('builds per-file review counts without reading patch bodies', () => { + const files = reviewFiles(['src/a.ts', 'src/new.ts']); + files[1]!.previousPath = 'src/old.ts'; + const draft = draftWithComments([ + draftComment('draft-a', 'src/a.ts'), + draftComment('stale-renamed', 'src/old.ts'), + draftComment('removed', 'src/removed.ts'), + ]); + const map = prReviewMapByPath({ + draft, + files, + findings: [ + { + sourceId: 'finding-minor', + severity: 'minor', + path: 'src/a.ts', + line: null, + summary: 'Minor finding', + suggestedFix: 'Fix it.', + reason: 'unanchorable', + }, + { + sourceId: 'finding-critical', + severity: 'critical', + path: 'src/a.ts', + line: null, + summary: 'Critical finding', + suggestedFix: 'Fix it first.', + reason: 'unanchorable', + }, + ], + staleCommentIds: new Set(['stale-renamed', 'removed']), + unresolvedThreads: [ + reviewThread('thread-a', 'src/a.ts'), + reviewThread('thread-renamed', 'src/old.ts'), + reviewThread('thread-removed', 'src/removed.ts'), + ], + }); + + expect(map.get('src/a.ts')).toEqual({ + draftCount: 1, + findingCount: 2, + highestFindingSeverity: 'critical', + path: 'src/a.ts', + staleDraftCount: 0, + unresolvedThreadCount: 1, + }); + expect(map.get('src/new.ts')).toEqual({ + draftCount: 1, + findingCount: 0, + highestFindingSeverity: null, + path: 'src/new.ts', + staleDraftCount: 1, + unresolvedThreadCount: 1, + }); + }); + it('clears only the editor instance whose mutation completed', () => { const submitted = { body: 'First draft', commentId: 'comment-1', token: 1 }; const newerComment = { diff --git a/web/src/features/pr-review/GitHubPrReview.tsx b/web/src/features/pr-review/GitHubPrReview.tsx index a94b9608..44f28c97 100644 --- a/web/src/features/pr-review/GitHubPrReview.tsx +++ b/web/src/features/pr-review/GitHubPrReview.tsx @@ -44,6 +44,7 @@ import { mergePatchResults, mutationErrorMessage, prDetail, + prReviewMapByPath, reviewFileStats, reviewPatchQuerySettled, summaryLabel, @@ -256,6 +257,23 @@ export function GitHubPrReview({ ), [blockedCommentIds, composer, draft, reviewThreads], ); + const reviewMapByPath = useMemo( + () => + prReviewMapByPath({ + draft, + files: fileList, + findings: reviewRecord?.reportOnlyFindings ?? [], + staleCommentIds, + unresolvedThreads, + }), + [ + draft, + fileList, + reviewRecord?.reportOnlyFindings, + staleCommentIds, + unresolvedThreads, + ], + ); const fileStats = useMemo(() => reviewFileStats(files), [files]); const isDraftMutationPending = mutations.saveDraft.isPending || @@ -859,6 +877,7 @@ export function GitHubPrReview({ onSelectedLinesChange={onSelectionChange} patchError={patchErrorMessage} renderAnnotation={renderAnnotation} + reviewMapByPath={reviewMapByPath} selectedLines={ composer?.path === activePath ? composer.selection : null } diff --git a/web/src/features/pr-review/PrReviewDiffPane.tsx b/web/src/features/pr-review/PrReviewDiffPane.tsx index ab9e3595..cd71d639 100644 --- a/web/src/features/pr-review/PrReviewDiffPane.tsx +++ b/web/src/features/pr-review/PrReviewDiffPane.tsx @@ -2,7 +2,11 @@ import type { SelectedLineRange } from '@pierre/diffs/react'; import type { ReactNode } from 'react'; import { MiniEmpty } from '../../components/ui'; import { MultiFileView } from '../diff-viewer/MultiFileView'; -import type { DiffFilePatch, DiffReviewAnnotation } from '../diff-viewer/types'; +import type { + DiffFilePatch, + DiffReviewAnnotation, + FileReviewMapEntry, +} from '../diff-viewer/types'; import type { ReviewSourceSnapshot } from '../../../../shared/review-source'; import { PrReviewFindingsSidebar, @@ -23,6 +27,7 @@ export function PrReviewDiffPane({ patchError, renderAnnotation, selectedLines, + reviewMapByPath, source, title, }: { @@ -39,6 +44,7 @@ export function PrReviewDiffPane({ patchError: string | null; renderAnnotation: (annotation: DiffReviewAnnotation) => ReactNode; selectedLines: SelectedLineRange | null; + reviewMapByPath: ReadonlyMap; source: ReviewSourceSnapshot; title: string; }) { @@ -74,6 +80,7 @@ export function PrReviewDiffPane({ onSelectedLinesChange={onSelectedLinesChange} patchError={patchError} renderAnnotation={renderAnnotation} + reviewMapByPath={reviewMapByPath} selectedLines={selectedLines} source={source} title={title} diff --git a/web/src/features/pr-review/review-view-model.ts b/web/src/features/pr-review/review-view-model.ts index 1b9248a7..a0fbd273 100644 --- a/web/src/features/pr-review/review-view-model.ts +++ b/web/src/features/pr-review/review-view-model.ts @@ -6,6 +6,7 @@ import { type GitHubPrReviewDraftComment, type GitHubPullRequest, type GitHubPullRequestReviewThread, + type PrReviewReportOnlyFinding, } from '../../api'; import type { GitHubPrReviewDraftResponse, @@ -14,7 +15,11 @@ import type { } from '../../api'; import { queryErrorMessage } from '../../lib/query'; import { patchHasContent } from '../diff-viewer/helpers'; -import type { DiffFilePatch, DiffReviewAnnotation } from '../diff-viewer/types'; +import type { + DiffFilePatch, + DiffReviewAnnotation, + FileReviewMapEntry, +} from '../diff-viewer/types'; import type { PullRequestFilePatchQueryState } from './queries'; import { commentInputFromSelection, @@ -27,6 +32,102 @@ import { threadPath, } from './review-ui-helpers'; +const findingSeverityRank = { + nit: 0, + minor: 1, + major: 2, + critical: 3, +} as const; + +export function prReviewMapByPath({ + draft, + files, + findings, + staleCommentIds, + unresolvedThreads, +}: { + draft: GitHubPrReviewDraft | null; + files: DiffFilePatch[]; + findings: PrReviewReportOnlyFinding[]; + staleCommentIds: ReadonlySet; + unresolvedThreads: GitHubPullRequestReviewThread[]; +}) { + const result = new Map(); + const currentPathByAlias = reviewMapPathAliases(files); + for (const file of files) { + result.set(file.path, emptyFileReviewMapEntry(file.path)); + } + + for (const thread of unresolvedThreads) { + if (thread.isResolved) continue; + const path = resolveReviewMapPath( + threadPath(thread), + result, + currentPathByAlias, + ); + if (!path) continue; + const entry = result.get(path); + if (entry) entry.unresolvedThreadCount += 1; + } + for (const comment of draft?.comments ?? []) { + const path = resolveReviewMapPath(comment.path, result, currentPathByAlias); + if (!path) continue; + const entry = result.get(path); + if (!entry) continue; + entry.draftCount += 1; + if (staleCommentIds.has(comment.id)) entry.staleDraftCount += 1; + } + for (const finding of findings) { + const path = resolveReviewMapPath(finding.path, result, currentPathByAlias); + if (!path) continue; + const entry = result.get(path); + if (!entry) continue; + entry.findingCount += 1; + if ( + entry.highestFindingSeverity === null || + findingSeverityRank[finding.severity] > + findingSeverityRank[entry.highestFindingSeverity] + ) { + entry.highestFindingSeverity = finding.severity; + } + } + return result; +} + +function emptyFileReviewMapEntry(path: string): FileReviewMapEntry { + return { + draftCount: 0, + findingCount: 0, + highestFindingSeverity: null, + path, + staleDraftCount: 0, + unresolvedThreadCount: 0, + }; +} + +function reviewMapPathAliases(files: DiffFilePatch[]) { + const aliases = new Map(); + for (const file of files) { + if (!file.previousPath || file.previousPath === file.path) continue; + const current = aliases.get(file.previousPath); + aliases.set( + file.previousPath, + current === undefined || current === file.path ? file.path : null, + ); + } + return aliases; +} + +function resolveReviewMapPath( + path: string | null, + entries: ReadonlyMap, + aliases: ReadonlyMap, +) { + if (!path) return null; + if (entries.has(path)) return path; + return aliases.get(path) ?? null; +} + export function mergePatchResults( files: DiffFilePatch[], patchQueryByPath: Map, From 9da4b65c59caff2f90c14d8134837c42376efa30 Mon Sep 17 00:00:00 2001 From: syn Date: Sat, 18 Jul 2026 10:32:53 -0500 Subject: [PATCH 2/2] fix(review): stabilize cursor identity and ordering --- shared/review-navigation.ts | 57 +++++++++++++++------------ src/review-navigation.test.ts | 74 +++++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 24 deletions(-) diff --git a/shared/review-navigation.ts b/shared/review-navigation.ts index bd987531..08483292 100644 --- a/shared/review-navigation.ts +++ b/shared/review-navigation.ts @@ -6,6 +6,7 @@ export type ReviewNavigationFile = { }; type ReviewNavigationItemBase = { + /** Globally stable within its target kind. Hunk ids are file-local instead. */ id: string; path: string; summary?: string | null; @@ -14,6 +15,8 @@ type ReviewNavigationItemBase = { export type ReviewHunkNavigationItem = ReviewNavigationItemBase & { kind: 'hunk'; + /** Hunk ids need only be unique within `path`; normalized keys include both. */ + id: string; oldStart?: number | null; newStart?: number | null; }; @@ -58,7 +61,8 @@ export type ReviewNavigationTarget = { path: string; requestedPath: string; previousPath: string | null; - fileIndex: number; + /** File position in the order used to produce this target projection. */ + orderIndex: number; position: number; summary: string | null; severity: ReviewFindingSeverity | null; @@ -132,12 +136,12 @@ export function createReviewNavigationModel( ); const seenKeys = new Set(); const targets: ReviewNavigationTarget[] = canonicalFilePaths.map( - (path, fileIndex) => ({ - fileIndex, + (path, orderIndex) => ({ id: path, key: targetKey('file', path), kind: 'file', missing: false, + orderIndex, path, position: 0, previousPath: previousPathByPath.get(path) ?? null, @@ -152,14 +156,14 @@ export function createReviewNavigationModel( const unavailableTargets: ReviewNavigationTarget[] = []; for (const [inputIndex, item] of (input.items ?? []).entries()) { if (item.kind === 'review-thread' && item.resolved) continue; - const key = targetKey(item.kind, item.id); - if (seenKeys.has(key)) continue; - seenKeys.add(key); const resolvedPath = resolvePath(item.path, fileIndexByPath, aliasToPath); const missing = resolvedPath === null; const path = resolvedPath ?? item.path; + const key = navigationItemKey(item, path); + if (seenKeys.has(key)) continue; + seenKeys.add(key); const target: ReviewNavigationTarget = { - fileIndex: resolvedPath + orderIndex: resolvedPath ? (fileIndexByPath.get(resolvedPath) ?? canonicalFilePaths.length) : canonicalFilePaths.length, id: item.id, @@ -180,7 +184,7 @@ export function createReviewNavigationModel( if (missing) unavailableTargets.push(target); } - targets.sort(targetComparator(fileIndexByPath)); + targets.sort(targetComparator); const attentionTargets = targets .filter( ( @@ -242,7 +246,11 @@ export function reviewCursorTargets( const filter = options.filter; return source .filter((target) => matchesFilter(target, kind, filter)) - .sort(targetComparator(fileIndexByPath)); + .map((target) => ({ + ...target, + orderIndex: fileIndexByPath.get(target.path) ?? fileOrder.length, + })) + .sort(targetComparator); } export function moveReviewCursor( @@ -377,19 +385,20 @@ function targetKey(kind: ReviewNavigationTargetKind, id: string) { return `${kind}:${id}`; } -function targetComparator(fileIndexByPath: ReadonlyMap) { - return (left: ReviewCursorTarget, right: ReviewCursorTarget) => { - const leftFileIndex = - fileIndexByPath.get(left.path) ?? Number.MAX_SAFE_INTEGER; - const rightFileIndex = - fileIndexByPath.get(right.path) ?? Number.MAX_SAFE_INTEGER; - return ( - leftFileIndex - rightFileIndex || - left.position - right.position || - cursorKindOrder(left) - cursorKindOrder(right) || - left.key.localeCompare(right.key) - ); - }; +function navigationItemKey(item: ReviewNavigationItem, resolvedPath: string) { + if (item.kind === 'hunk') { + return `hunk:${JSON.stringify([resolvedPath, item.id])}`; + } + return targetKey(item.kind, item.id); +} + +function targetComparator(left: ReviewCursorTarget, right: ReviewCursorTarget) { + return ( + left.orderIndex - right.orderIndex || + left.position - right.position || + cursorKindOrder(left) - cursorKindOrder(right) || + left.key.localeCompare(right.key) + ); } function cursorKindOrder(target: ReviewCursorTarget) { @@ -451,8 +460,8 @@ function nearestTargetIndex( let bestScore: readonly number[] | null = null; for (const [index, target] of targets.entries()) { const sameFile = targetMatchesPath(target, anchor.path) ? 0 : 1; - const fileDistance = Math.abs(target.fileIndex - anchor.fileIndex); - const preferFollowingFile = target.fileIndex >= anchor.fileIndex ? 0 : 1; + const fileDistance = Math.abs(target.orderIndex - anchor.orderIndex); + const preferFollowingFile = target.orderIndex >= anchor.orderIndex ? 0 : 1; const positionDistance = Math.abs(target.position - anchor.position); const preferFollowingPosition = target.position >= anchor.position ? 0 : 1; const score = [ diff --git a/src/review-navigation.test.ts b/src/review-navigation.test.ts index 9fc927f5..3ae624ba 100644 --- a/src/review-navigation.test.ts +++ b/src/review-navigation.test.ts @@ -74,6 +74,38 @@ describe('review navigation model', () => { ]); }); + it('scopes file-local hunk ids without ambiguous key collisions', () => { + const model = createReviewNavigationModel({ + files: [{ path: 'src/a:b.ts' }, { path: 'src/a.ts' }], + items: [ + { + kind: 'hunk', + id: 'shared:hunk', + path: 'src/a:b.ts', + newStart: 1, + }, + { + kind: 'hunk', + id: 'shared:hunk', + path: 'src/a.ts', + newStart: 2, + }, + ], + }); + const targets = reviewCursorTargets(model, 'hunk'); + + expect(targets).toHaveLength(2); + expect(targets.map((target) => target.id)).toEqual([ + 'shared:hunk', + 'shared:hunk', + ]); + expect(new Set(targets.map((target) => target.key)).size).toBe(2); + expect(targets.map((target) => target.key)).toEqual([ + 'hunk:["src/a:b.ts","shared:hunk"]', + 'hunk:["src/a.ts","shared:hunk"]', + ]); + }); + it('resolves targets on previous paths to renamed files', () => { const model = createReviewNavigationModel({ files: [{ path: 'src/new.ts', previousPath: 'src/old.ts' }], @@ -286,6 +318,48 @@ describe('review navigation model', () => { target: { path: 'src/c.ts' }, }); }); + + it('falls forward in guided order when the active target is removed', () => { + const before = createReviewNavigationModel({ + files: [{ path: 'src/a.ts' }, { path: 'src/b.ts' }, { path: 'src/c.ts' }], + guidedOrder: ['src/c.ts', 'src/b.ts', 'src/a.ts'], + items: [ + { kind: 'local-draft', id: 'draft-a', path: 'src/a.ts', line: 1 }, + { kind: 'local-draft', id: 'draft-b', path: 'src/b.ts', line: 1 }, + { kind: 'local-draft', id: 'draft-c', path: 'src/c.ts', line: 1 }, + ], + }); + const after = createReviewNavigationModel({ + files: [{ path: 'src/a.ts' }, { path: 'src/c.ts' }], + guidedOrder: ['src/c.ts', 'src/a.ts'], + items: [ + { kind: 'local-draft', id: 'draft-a', path: 'src/a.ts', line: 1 }, + { kind: 'local-draft', id: 'draft-c', path: 'src/c.ts', line: 1 }, + ], + }); + const previousTargets = reviewCursorTargets(before, 'local-draft', { + order: 'guided', + }); + const nextTargets = reviewCursorTargets(after, 'local-draft', { + order: 'guided', + }); + + expect(previousTargets.map((target) => target.path)).toEqual([ + 'src/c.ts', + 'src/b.ts', + 'src/a.ts', + ]); + expect( + reconcileReviewCursor( + previousTargets, + nextTargets, + 'local-draft:draft-b', + ), + ).toMatchObject({ + resolution: 'nearest', + target: { id: 'draft-a', orderIndex: 1, path: 'src/a.ts' }, + }); + }); }); function fixture(): ReviewNavigationInput {