Skip to content

Commit e1efea8

Browse files
authored
feat(review): file comments in the diff + unified click-to-highlight comment UX (#973)
* feat(review): file comments in diff + unified click-to-highlight comment UX Render file-scoped comments inline in the code review UI and overhaul the comment-card UX for consistency. File comments - Single-file view: a full-text banner below the file path. - All-files view: rendered in the file header (expanded files only) — Pierre's virtualized custom header suppresses the body-prefix slot, and its measured item heights make a taller header safe. Guided-review external annotations can now drop the line-1 anchor hack. Click-to-highlight - Clicking a comment (inline card or sidebar) replays its stored line range as the controlled selection highlight; clicking it again clears it. - Scroll/navigate is sidebar- and findings-list-only (token-based signal) so clicking a comment in the diff never moves the viewport. Unified comment cards - Shared CommentMeta identity row (badges + author + relative time) across the inline, sidebar, and file-banner cards (were three hand-rolled headers). - Shared FileNameChip (basename, full path on hover) replaces the literal "file" badge everywhere. - Force the app sans font on cards (inline was inheriting the diff's monospace), consistent selected ring, spacing, and absolute hover actions so timestamps align across surfaces. Shared helpers: annotationScope (PR-scope / file-scope / line-range projection) and fileName (basename). Plumbs createdAt through DiffAnnotationMetadata so the inline cards can show timestamps. * feat(review): unify comment action row (edit · copy · delete) Shared CommentActions component used by the inline diff card, the file-comment banner, and the sidebar — bottom-aligned, hover-revealed, order left→right edit · copy · delete (delete/close furthest right). - Inline + banner: actions moved from the top (where they overlapped the timestamp) to the bottom, and gained the copy button. - Sidebar: now uses the same component (copy + delete) — same icons, order, and styling. - Read-only (externally-sourced) file comments show copy only. - Extract commentCopyText() so every card's copy produces identical, self-describing text (location prefix + body + reasoning); drops the format that was hand-built in the sidebar and the agent-job panel. - Rename PRCommentsTab's local CommentActions -> PRCommentLinkActions to avoid the name clash with the shared component. - Remove the now-dead .review-comment-action(s) CSS and CommentMeta's trailing slot. * fix(review): address PR review findings - all-files: let an active compose own the line highlight (pendingSelection guard) so clicking/deselecting a comment mid-compose no longer clobbers the toolbar's range — mirrors single-file's `pendingSelection ?? annotationRange`. - clear scrollTargetAnnotation on inline select so a remount (Refresh / base switch) doesn't re-scroll back to a stale sidebar/findings navigate target. - inline diff cards: hide edit/delete for external (source-set) findings (matches the file-comment card) and copy with the file:line location prefix via a precomputed metadata.copyText (commentCopyText), unifying the copy text. - add decorations + createdAt to the file-comment refresh signature so an external PATCH changing only those repaints the all-files header. - wrap finding reasoning text (overflow-wrap/word-break) so long file:line refs no longer bleed past the card edge. * refactor(review): dedupe line-annotation projection + close signature gaps Self-review follow-ups to the PR-feedback fixes: - Extract lineAnnotationMetadata() — the all-files and single-file diff surfaces built the identical 14-field DiffAnnotationMetadata; now one source of truth. - Add reviewProfileLabel + source + createdAt to the LINE-scope refresh signature: the unified CommentMeta now renders those on inline cards too, so an external PATCH changing only them must repaint the item (same gap that was just fixed for the file-comment tuple). - Wrap the agent-job panel's finding text + reasoning (overflow-wrap) so long file:line refs don't bleed past the panel edge, matching the inline card fix. * fix(review): zero inter-file gap + stop file-path descender clipping - all-files: set CodeView layout gap to 0 (Pierre default was 8) so files sit flush; sticky headers already mark file boundaries. Keep default edge padding. - FileHeader: path text leading-none -> leading-normal so the overflow-hidden ellipsis box has room for descenders (g/y/p were clipped by the box bottom). * fix(review): address 2nd-pass review — header re-measure, delete, highlight - all-files file comments: re-measure the CodeView item when a comment expands/collapses/edits (FileCommentCard -> onHeightChange -> refreshItem) so Pierre repositions following items instead of overlapping them. Fixes the dynamic header-height bug; the remaining static sticky-offset drift is bounded by the new banner cap and stays the accepted trade-off. - cap the expanded comment body (max-h + overflow) so a long guided-review note doesn't fill the viewport / inflate the sticky header. - delete is now available on every surface (inline, banner) including external source-set findings; edit stays gated on !source. - restore the line highlight when a compose ends: the all-files pendingSelection effect now re-derives the selected comment's range instead of clearing (mirrors single-file's pendingSelection ?? annotationRange). - add reasoning to the file-comment refresh signature (copyText includes it). - guard handleNavigateToAnnotation with annotationMatchesPrScope so a stale sidebar/finding row can't navigate to an out-of-scope annotation. * refactor(review): share annotation→highlight derivation + smoother re-measure Self-review follow-ups: - Extract lineSelectionForAnnotation() — the compose-end restore and the selection-replay effect built the same annotation→CodeViewLineSelection by hand; share it so they can't diverge. - Re-measure file-comment height via useLayoutEffect (before paint) instead of useEffect, so an expand/collapse lands without a one-frame overlap. * fix(review): ignore navigate to missing annotation + re-seed on scope switch - handleNavigateToAnnotation: return early when the annotation id is gone (deleted) as well as out-of-scope, so a stale row can't leave an orphan selection (ring with nothing to scroll to). Dismissed agent-panel rows are already non-clickable; this is the belt-and-suspenders for any other path. - fileSetKey now includes prUrl/prDiffScope so a pure layer<->full-stack scope switch (same file set) remounts and re-seeds annotations through the current scope filter — the incremental sync bails on an unchanged annotations ref and can't otherwise detect the filter change. * fix(review): allow editing external/source findings inline again Drop the editable = !source gate on inline and file-comment cards. main always showed edit for external findings (it routes through updateExternalAnnotation), so gating it was a regression. Edit, delete, and copy are now all available on every comment surface regardless of source. * refactor(review): drop redundant null-check after navigate guard annotation is guaranteed non-null past the missing/out-of-scope early return.
1 parent 3efd996 commit e1efea8

23 files changed

Lines changed: 757 additions & 231 deletions

bun.lock

Lines changed: 3 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

packages/review-editor/App.tsx

Lines changed: 33 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,8 @@ import {
8585
REVIEW_ALL_FILES_PANEL_ID,
8686
REVIEW_CODE_NAV_PANEL_ID,
8787
} from './dock/reviewPanelTypes';
88-
import type { DiffFile } from './types';
88+
import type { DiffFile, AnnotationScrollTarget } from './types';
89+
import { annotationMatchesPrScope } from './utils/annotationScope';
8990
import type { DiffOption, WorktreeInfo, GitContext } from '@plannotator/shared/types';
9091
import type { PRMetadata } from '@plannotator/shared/pr-types';
9192
import type { PRDiffScope, PRDiffScopeOption, PRStackInfo, PRStackTree } from '@plannotator/shared/pr-stack';
@@ -121,6 +122,11 @@ const ReviewApp: React.FC = () => {
121122
const [activeFileIndex, setActiveFileIndex] = useState(0);
122123
const [annotations, setAnnotations] = useState<CodeAnnotation[]>([]);
123124
const [selectedAnnotationId, setSelectedAnnotationId] = useState<string | null>(null);
125+
// Sidebar-initiated "scroll to this comment" signal. The token bumps on every
126+
// sidebar click so re-selecting the same comment re-navigates. Selecting a
127+
// comment in the diff sets selectedAnnotationId but NOT this — so it never
128+
// moves the viewport.
129+
const [scrollTargetAnnotation, setScrollTargetAnnotation] = useState<AnnotationScrollTarget | null>(null);
124130
const [isAllFilesActive, setIsAllFilesActive] = useState(false);
125131
// Mirror ref: handlers captured by Pierre slot portals (which only republish
126132
// on item version bumps) and early-declared callbacks read the CURRENT value
@@ -1511,30 +1517,39 @@ const ReviewApp: React.FC = () => {
15111517
// handler is baked into Pierre slot portals, which only republish on item
15121518
// version bumps — a stale captured value would yank the user out of the
15131519
// all-files tab into the single-file panel when they click an annotation.
1520+
// Inline-card selection: toggle the highlight + ring only. No scroll, no file
1521+
// switch — the clicked card is already on screen. Clicking the selected card
1522+
// again (or a null id) clears it.
15141523
const handleSelectAnnotation = useCallback((id: string | null) => {
1524+
// An inline selection supersedes any pending sidebar/findings navigate target,
1525+
// so a later remount (Refresh / base switch) doesn't re-scroll back to it.
1526+
setScrollTargetAnnotation(null);
1527+
setSelectedAnnotationId(prev => (!id || prev === id ? null : id));
1528+
}, []);
1529+
1530+
// Sidebar navigation: select AND scroll-to the comment (DiffsHub "set +
1531+
// scroll"). The token bump re-fires the panels' scroll effect even when the
1532+
// same comment is clicked twice; in single-file mode it switches to the
1533+
// owning file first so the scroll target exists.
1534+
const handleNavigateToAnnotation = useCallback((id: string | null) => {
15151535
if (!id) {
15161536
setSelectedAnnotationId(null);
15171537
return;
15181538
}
1519-
1520-
// Find the annotation
15211539
const annotation = allAnnotationsRef.current.find(a => a.id === id);
1522-
if (!annotation) {
1523-
setSelectedAnnotationId(id);
1540+
// Ignore navigation to an annotation that's gone (deleted) or filtered out of
1541+
// the active PR/diff-scope — there's nothing in the current diff to scroll to
1542+
// or highlight, so don't fake a selection.
1543+
if (!annotation || !annotationMatchesPrScope(annotation, prMetadata?.url, prDiffScope)) {
15241544
return;
15251545
}
1526-
1527-
// In all-files mode, just set the selection — the panel's scroll-to-annotation
1528-
// effect handles expanding and scrolling. In single-file mode, switch to the file.
15291546
if (!isAllFilesActiveRef.current) {
15301547
const fileIndex = files.findIndex(f => f.path === annotation.filePath);
1531-
if (fileIndex !== -1) {
1532-
handleFileSwitch(fileIndex);
1533-
}
1548+
if (fileIndex !== -1) handleFileSwitch(fileIndex);
15341549
}
1535-
15361550
setSelectedAnnotationId(id);
1537-
}, [files, handleFileSwitch]);
1551+
setScrollTargetAnnotation(prev => ({ id, token: (prev?.token ?? 0) + 1 }));
1552+
}, [files, handleFileSwitch, prMetadata, prDiffScope]);
15381553

15391554
// Diff context bundled into local-mode feedback headers so the receiving
15401555
// agent knows which diff the annotations are anchored to. Uses committedBase
@@ -1596,6 +1611,7 @@ const ReviewApp: React.FC = () => {
15961611
allAnnotations,
15971612
externalAnnotations,
15981613
selectedAnnotationId,
1614+
scrollTargetAnnotation,
15991615
pendingSelection,
16001616
onLineSelection: handleLineSelection,
16011617
onAddAnnotation: handleAddAnnotation,
@@ -1604,6 +1620,7 @@ const ReviewApp: React.FC = () => {
16041620
onAddFileCommentForFile: handleAddFileCommentForFile,
16051621
onEditAnnotation: handleEditAnnotation,
16061622
onSelectAnnotation: handleSelectAnnotation,
1623+
onNavigateToAnnotation: handleNavigateToAnnotation,
16071624
onDeleteAnnotation: handleDeleteAnnotation,
16081625
viewedFiles,
16091626
onToggleViewed: handleToggleViewed,
@@ -1654,9 +1671,9 @@ const ReviewApp: React.FC = () => {
16541671
diffLineDiffType, diffShowLineNumbers, diffShowBackground,
16551672
diffExpandUnchanged, diffFontFamily, diffFontSize, activeDiffBase, committedBase, feedbackDiffContext, prReviewScopeLabel, prDiffScope, agentCwd,
16561673
allAnnotations, externalAnnotations,
1657-
selectedAnnotationId, pendingSelection, handleLineSelection,
1674+
selectedAnnotationId, scrollTargetAnnotation, pendingSelection, handleLineSelection,
16581675
handleAddAnnotation, handleAddFileComment, handleAddFileCommentForFile, handleEditAnnotation,
1659-
handleSelectAnnotation, handleDeleteAnnotation, viewedFiles,
1676+
handleSelectAnnotation, handleNavigateToAnnotation, handleDeleteAnnotation, viewedFiles,
16601677
handleToggleViewed, stagedFiles, stagingFile, stageFile,
16611678
canStageFiles, stageError, isSearchPending, debouncedSearchQuery,
16621679
activeFileSearchMatches, activeSearchMatchId, activeSearchMatch, searchMatches,
@@ -2575,6 +2592,7 @@ const ReviewApp: React.FC = () => {
25752592
files={files}
25762593
selectedAnnotationId={selectedAnnotationId}
25772594
onSelectAnnotation={handleSelectAnnotation}
2595+
onNavigateToAnnotation={handleNavigateToAnnotation}
25782596
onDeleteAnnotation={handleDeleteAnnotation}
25792597
feedbackMarkdown={feedbackMarkdown}
25802598
width={panelResize.width}

packages/review-editor/components/AITab.tsx

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import type { AIChatEntry, PendingPermission } from '../hooks/useAIChat';
33
import { renderChatMarkdown } from '../utils/renderChatMarkdown';
44
import { formatLineRange } from '../utils/formatLineRange';
55
import { formatRelativeTime } from '../utils/formatRelativeTime';
6+
import { FileNameChip } from './FileNameChip';
67
import { SparklesIcon } from '@plannotator/ui/components/SparklesIcon';
78
import { CountBadge } from './CountBadge';
89
import { CopyButton } from './CopyButton';
@@ -352,10 +353,8 @@ const QAPair = memo<{
352353
{formatLineRange(question.lineStart, question.lineEnd)}
353354
</button>
354355
)}
355-
{scope === 'file' && (
356-
<span className="text-[9px] font-semibold uppercase tracking-wider px-1.5 py-0.5 rounded bg-primary/10 text-primary">
357-
file
358-
</span>
356+
{scope === 'file' && question.filePath && (
357+
<FileNameChip path={question.filePath} />
359358
)}
360359
</div>
361360
<span className="text-[10px] text-muted-foreground/50">

0 commit comments

Comments
 (0)