From 0e4d3019250fb9f7a2ca620a6e980fe522c07c65 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 7 May 2026 18:54:06 -0400 Subject: [PATCH] Review UI refinements: separator styling and header layout (#683) * Collapse viewed files in all-files review * Style diff hunk separators to be slimmer and subtler Reduce separator height from 32px to 24px, use theme-integrated semi-transparent backgrounds via --diffs-bg-separator-override, and fade text/buttons with opacity for a less visually heavy bar. * Refine review header layout Move sidebar toggles to far right, options menu to their left. Add divider between file tree button and repo label. Remove worktree pill from header (still accessible via menu). * Fix gutter drag selecting only one line Switch from deprecated enableHoverUtility + renderHoverUtility to enableGutterUtility + onGutterUtilityClick. The old API never entered Pierre's gutterSelecting mode, so dragging the gutter button always reported a single line. The new API receives the full drag range. Closes #679 --- packages/review-editor/App.tsx | 61 +++++++++--------- .../components/AllFilesDiffView.tsx | 63 +++++++++---------- .../review-editor/components/DiffViewer.tsx | 37 +++-------- .../review-editor/components/LazyFileDiff.tsx | 3 - .../review-editor/hooks/usePierreTheme.ts | 49 +++++++++++++++ packages/review-editor/index.css | 26 -------- 6 files changed, 115 insertions(+), 124 deletions(-) diff --git a/packages/review-editor/App.tsx b/packages/review-editor/App.tsx index 220915fa..dbb85ebd 100644 --- a/packages/review-editor/App.tsx +++ b/packages/review-editor/App.tsx @@ -1712,30 +1712,25 @@ const ReviewApp: React.FC = () => {
{shouldShowFileTree && ( - + <> + +
+ )} {prMetadata ? (
- {(gitContext || agentCwd) && ( - - )} @@ -1983,6 +1978,18 @@ const ReviewApp: React.FC = () => {
+ setOpenSettingsMenu(true)} + onOpenExport={() => setShowExportModal(true)} + onToggleFileTree={() => setIsFileTreeOpen(prev => !prev)} + onToggleSidebar={() => reviewSidebar.isOpen ? reviewSidebar.close() : reviewSidebar.open()} + isFileTreeOpen={isFileTreeOpen} + isSidebarOpen={reviewSidebar.isOpen} + appVersion={appVersion} + /> + +
+ {/* Sidebar tab toggles */} )} - -
- - setOpenSettingsMenu(true)} - onOpenExport={() => setShowExportModal(true)} - onToggleFileTree={() => setIsFileTreeOpen(prev => !prev)} - onToggleSidebar={() => reviewSidebar.isOpen ? reviewSidebar.close() : reviewSidebar.open()} - isFileTreeOpen={isFileTreeOpen} - isSidebarOpen={reviewSidebar.isOpen} - appVersion={appVersion} - />
diff --git a/packages/review-editor/components/AllFilesDiffView.tsx b/packages/review-editor/components/AllFilesDiffView.tsx index 2b0d13fd..6b9cd94e 100644 --- a/packages/review-editor/components/AllFilesDiffView.tsx +++ b/packages/review-editor/components/AllFilesDiffView.tsx @@ -154,6 +154,24 @@ export const AllFilesDiffView: React.FC = ({ }); }, [activeFilePath]); + const collapseFile = useCallback((filePath: string) => { + if (!collapsedFiles.has(filePath)) { + collapseHistory.current.push(filePath); + setCollapsedFiles(prev => { + const next = new Set(prev); + next.add(filePath); + return next; + }); + } + if (activeFilePath === filePath) setActiveFilePath(null); + }, [activeFilePath, collapsedFiles]); + + const toggleViewedAndCollapse = useCallback((filePath: string) => { + const isCurrentlyViewed = viewedFiles.has(filePath); + onToggleViewed?.(filePath); + if (!isCurrentlyViewed) collapseFile(filePath); + }, [collapseFile, onToggleViewed, viewedFiles]); + const setHeaderRef = useCallback((path: string, el: HTMLDivElement | null) => { if (el) headerRefs.current.set(path, el); else headerRefs.current.delete(path); @@ -243,17 +261,7 @@ export const AllFilesDiffView: React.FC = ({ if (e.key === 'v' && currentPath) { e.preventDefault(); - const isCurrentlyViewed = viewedFiles.has(currentPath); - onToggleViewed?.(currentPath); - if (!isCurrentlyViewed) { - collapseHistory.current.push(currentPath); - setCollapsedFiles(prev => { - const next = new Set(prev); - next.add(currentPath); - return next; - }); - setActiveFilePath(null); - } + toggleViewedAndCollapse(currentPath); return; } @@ -287,7 +295,7 @@ export const AllFilesDiffView: React.FC = ({ }; window.addEventListener('keydown', handler); return () => window.removeEventListener('keydown', handler); - }, [isActive, sortedFiles, collapsedFiles, activeFilePath, toggleCollapse, viewedFiles, onToggleViewed, canStageFiles, onStage, onAddFileComment]); + }, [isActive, sortedFiles, collapsedFiles, activeFilePath, toggleCollapse, toggleViewedAndCollapse, canStageFiles, onStage, onAddFileComment]); // Click-and-drag line selection in diff content useEffect(() => { @@ -391,7 +399,7 @@ export const AllFilesDiffView: React.FC = ({ filePath={file.path} patch={file.patch} isViewed={viewedFiles.has(file.path)} - onToggleViewed={onToggleViewed ? () => onToggleViewed(file.path) : undefined} + onToggleViewed={onToggleViewed ? () => toggleViewedAndCollapse(file.path) : undefined} isStaged={stagedFiles?.has(file.path)} isStaging={stagingFile === file.path} onStage={onStage ? () => onStage(file.path) : undefined} @@ -437,7 +445,15 @@ export const AllFilesDiffView: React.FC = ({ disableBackground, hunkSeparators: 'line-info', enableLineSelection: true, - enableHoverUtility: true, + enableGutterUtility: true, + onGutterUtilityClick: (range: SelectedLineRange) => { + if (activeFilePath === file.path) { + toolbarHostRef.current?.handleLineSelectionEnd(range); + } else { + pendingToolbarRange.current = range; + setActiveFilePath(file.path); + } + }, onLineSelectionEnd: (range: SelectedLineRange | null) => { if (range) { if (activeFilePath === file.path) { @@ -464,25 +480,6 @@ export const AllFilesDiffView: React.FC = ({ /> ); }} - renderHoverUtility={(getHoveredLine: () => { lineNumber: number; side: 'deletions' | 'additions' } | undefined) => ( - - )} /> )} diff --git a/packages/review-editor/components/DiffViewer.tsx b/packages/review-editor/components/DiffViewer.tsx index aef0c287..912737c1 100644 --- a/packages/review-editor/components/DiffViewer.tsx +++ b/packages/review-editor/components/DiffViewer.tsx @@ -37,8 +37,8 @@ interface PierreDiffContentProps { mergedAnnotations: DiffLineAnnotation[]; pendingSelection: SelectedLineRange | null; onLineSelectionEnd: (range: SelectedLineRange | null) => void; + onGutterUtilityClick: (range: SelectedLineRange) => void; renderAnnotation: (annotation: { side: string; lineNumber: number; metadata?: DiffAnnotationMetadata }) => React.ReactNode; - renderHoverUtility: (getHoveredLine: () => { lineNumber: number; side: 'deletions' | 'additions' } | undefined) => React.ReactNode; onTokenClick?: (props: DiffTokenEventBaseProps, event: MouseEvent) => void; onTokenEnter?: (props: DiffTokenEventBaseProps, event: PointerEvent) => void; onTokenLeave?: (props: DiffTokenEventBaseProps, event: PointerEvent) => void; @@ -57,8 +57,8 @@ const PierreDiffContent = React.memo(({ mergedAnnotations, pendingSelection, onLineSelectionEnd, + onGutterUtilityClick, renderAnnotation, - renderHoverUtility, onTokenClick, onTokenEnter, onTokenLeave, @@ -79,7 +79,8 @@ const PierreDiffContent = React.memo(({ disableBackground, hunkSeparators: 'line-info', enableLineSelection: true, - enableHoverUtility: true, + enableGutterUtility: true, + onGutterUtilityClick, onLineSelectionEnd, onTokenClick, onTokenEnter, @@ -88,7 +89,6 @@ const PierreDiffContent = React.memo(({ lineAnnotations={mergedAnnotations} selectedLines={pendingSelection || undefined} renderAnnotation={renderAnnotation} - renderHoverUtility={renderHoverUtility} /> ); }, (prev, next) => ( @@ -107,8 +107,8 @@ const PierreDiffContent = React.memo(({ prev.mergedAnnotations === next.mergedAnnotations && prev.pendingSelection === next.pendingSelection && prev.onLineSelectionEnd === next.onLineSelectionEnd && + prev.onGutterUtilityClick === next.onGutterUtilityClick && prev.renderAnnotation === next.renderAnnotation && - prev.renderHoverUtility === next.renderHoverUtility && prev.onTokenClick === next.onTokenClick && prev.onTokenEnter === next.onTokenEnter && prev.onTokenLeave === next.onTokenLeave @@ -494,29 +494,8 @@ export const DiffViewer: React.FC = ({ ); }, [filePath, onSelectAnnotation, handleEdit, onDeleteAnnotation, onClickAIMarker]); - // Render hover utility (+ button). - // Pierre manages visibility imperatively — it inserts/removes the slot - // container on line hover. We must always return a button; calling - // getHoveredLine() to gate rendering would return stale data because - // Pierre's hover state changes don't trigger React re-renders. - const renderHoverUtility = useCallback((getHoveredLine: () => { lineNumber: number; side: 'deletions' | 'additions' } | undefined) => { - return ( - - ); + const handleGutterUtilityClick = useCallback((range: SelectedLineRange) => { + toolbarHostRef.current?.handleLineSelectionEnd(range); }, []); useEffect(() => { @@ -613,8 +592,8 @@ export const DiffViewer: React.FC = ({ mergedAnnotations={mergedAnnotations} pendingSelection={pendingSelection} onLineSelectionEnd={handlePierreLineSelectionEnd} + onGutterUtilityClick={handleGutterUtilityClick} renderAnnotation={renderAnnotation} - renderHoverUtility={renderHoverUtility} onTokenClick={handleTokenClick} onTokenEnter={handleTokenEnter} onTokenLeave={handleTokenLeave} diff --git a/packages/review-editor/components/LazyFileDiff.tsx b/packages/review-editor/components/LazyFileDiff.tsx index ddf8e7aa..7760bd1a 100644 --- a/packages/review-editor/components/LazyFileDiff.tsx +++ b/packages/review-editor/components/LazyFileDiff.tsx @@ -16,7 +16,6 @@ interface LazyFileDiffProps { annotations: DiffLineAnnotation[]; selectedLines: SelectedLineRange | undefined; renderAnnotation: (annotation: DiffLineAnnotation) => React.ReactNode; - renderHoverUtility: (getHoveredLine: () => { lineNumber: number; side: 'deletions' | 'additions' } | undefined) => React.ReactNode; } function estimateHeight(fileDiff: FileDiffMetadata, diffStyle: 'split' | 'unified'): number { @@ -35,7 +34,6 @@ export const LazyFileDiff: React.FC = ({ annotations, selectedLines, renderAnnotation, - renderHoverUtility, }) => { const [mounted, setMounted] = useState(forceMount); const sentinelRef = useRef(null); @@ -114,7 +112,6 @@ export const LazyFileDiff: React.FC = ({ lineAnnotations={annotations} selectedLines={selectedLines} renderAnnotation={renderAnnotation} - renderHoverUtility={renderHoverUtility} /> ); diff --git a/packages/review-editor/hooks/usePierreTheme.ts b/packages/review-editor/hooks/usePierreTheme.ts index bfa4cb88..87b94cb2 100644 --- a/packages/review-editor/hooks/usePierreTheme.ts +++ b/packages/review-editor/hooks/usePierreTheme.ts @@ -68,6 +68,9 @@ export function usePierreTheme(options?: { fontFamily?: string; fontSize?: strin --diffs-dark-bg: ${bg}; --diffs-light-bg: ${bg}; --diffs-dark: ${fg}; --diffs-light: ${fg}; } pre, code { background-color: ${bg} !important; } + :host { --diffs-bg-separator-override: color-mix(in srgb, ${fg} 8%, ${bg}); } + [data-separator='line-info'], [data-separator='line-info-basic'] { height: 24px !important; } + [data-separator='line-info'] { margin-block: 4px !important; } `}; }); @@ -77,6 +80,8 @@ export function usePierreTheme(options?: { fontFamily?: string; fontSize?: strin const bg = styles.getPropertyValue('--background').trim(); const fg = styles.getPropertyValue('--foreground').trim(); const muted = styles.getPropertyValue('--muted').trim(); + const mutedFg = styles.getPropertyValue('--muted-foreground').trim(); + const border = styles.getPropertyValue('--border').trim(); const primary = styles.getPropertyValue('--primary').trim(); if (!bg || !fg) return; @@ -120,6 +125,50 @@ export function usePierreTheme(options?: { fontFamily?: string; fontSize?: strin text-underline-offset: 2px; cursor: pointer; } + + /* Separator bars — slimmer, semi-transparent, integrated with theme */ + :host { + --diffs-bg-separator-override: color-mix(in srgb, ${border || fg} 25%, ${bg}); + } + [data-separator='line-info'], + [data-separator='line-info-basic'] { + height: 24px !important; + } + [data-separator='line-info'] { + margin-block: 4px !important; + } + [data-separator-content] { + font-size: 11px !important; + color: ${mutedFg || fg} !important; + opacity: 0.7; + } + [data-separator-content]:hover { + opacity: 1; + } + [data-expand-button] { + min-width: 24px !important; + color: ${mutedFg || fg} !important; + opacity: 0.5; + } + [data-expand-button]:hover { + color: ${fg} !important; + opacity: 1; + } + [data-expand-index] [data-separator-wrapper] { + grid-template-columns: 24px auto !important; + } + [data-expand-index] [data-separator-wrapper][data-separator-multi-button] { + grid-template-columns: 24px 24px auto !important; + } + @media (pointer: fine) { + [data-separator='line-info'] [data-separator-wrapper] { + grid-template-columns: 26px auto !important; + } + [data-separator='line-info'] [data-separator-wrapper][data-separator-multi-button] { + grid-template-columns: 26px 26px auto !important; + } + } + ${fontCSS} `, }); diff --git a/packages/review-editor/index.css b/packages/review-editor/index.css index cd2f6ee7..6004cc23 100644 --- a/packages/review-editor/index.css +++ b/packages/review-editor/index.css @@ -241,32 +241,6 @@ diffs-container { background: oklch(from var(--primary) l c h / 0.3) !important; } -/* Hover utility button — matches Pierre's [data-utility-button] sizing */ -.hover-add-comment { - appearance: none; - display: flex; - align-items: center; - justify-content: center; - width: 1lh; - height: 1lh; - font-size: var(--diffs-font-size, 13px); - line-height: var(--diffs-line-height, 20px); - border: none; - border-radius: 4px; - background-color: var(--diffs-modified-base); - color: var(--diffs-bg); - fill: currentColor; - cursor: pointer; - position: relative; - z-index: 4; - padding: 0; - margin-right: calc(1ch - 1lh); -} - -.hover-add-comment:hover { - filter: brightness(1.2); -} - /* File tree styling */ .file-tree-item { display: flex;