From bd4bc1b706824355d17437223702a1f83de4a9ce Mon Sep 17 00:00:00 2001 From: Philip Zeyliger Date: Fri, 23 Jan 2026 00:28:35 +0000 Subject: [PATCH] shelley: improve Monaco diff view in PatchTool Prompt: When the patch tool uses monaco views to put in the conversation view, the monaco view needs to be collapsed to just "edited" sections (not expanded). And the monaco view needs to be exactly the height to show all the code but no higher. We're also having issues with too many monaco editors making things slow. Activate the monaco editors only when they're on screen for the user. Have a toggle between side-by-side and inline diff modes for the monaco thing, and have that affect the default by using local storage. Follow-up: The toggle is ugly. We don't need the text "Diff:" or how long the tool took; maybe we can put the toggle up in the tool header thingy? When the diff is long, the monaco thing doesn't expand to fit it all. I think this isn't interacting well with scroll to bottom behavior as things are being added. - Add lazy loading via IntersectionObserver - Monaco editors only initialize when scrolled into view, improving performance with many patches in a conversation - Collapse to edited sections by default using hideUnchangedRegions option - shows only changed lines with 2 lines of context - Auto-size editor height to fit content exactly (no max height cap) - Only set height once after diff computes to avoid scroll disruption - Add toggle button for side-by-side vs inline diff modes in header - Persist diff mode preference in localStorage - Remove "Diff:" label and execution time display from Monaco view - Add placeholder UI for lazy-loaded state Co-authored-by: Shelley --- ui/src/components/PatchTool.tsx | 386 ++++++++++++++++++++++---------- ui/src/styles.css | 62 +++++ 2 files changed, 335 insertions(+), 113 deletions(-) diff --git a/ui/src/components/PatchTool.tsx b/ui/src/components/PatchTool.tsx index f1c1ad59e5d8290ed0708d94e63777d5fe5e5ca9..f9b849eadbcd4e43e990ff1878e7466fb46d460b 100644 --- a/ui/src/components/PatchTool.tsx +++ b/ui/src/components/PatchTool.tsx @@ -3,15 +3,41 @@ import type * as Monaco from "monaco-editor"; import { LLMContent } from "../types"; import { isDarkModeActive } from "../services/theme"; -// Feature flag for Monaco diff view (set localStorage.setItem('shelley-use-monaco-diff', 'true') to enable) +// LocalStorage keys for preferences +const STORAGE_KEY_MONACO_ENABLED = "shelley-use-monaco-diff"; +const STORAGE_KEY_SIDE_BY_SIDE = "shelley-diff-side-by-side"; + +// Feature flag for Monaco diff view function useMonacoDiff(): boolean { try { - return localStorage.getItem("shelley-use-monaco-diff") === "true"; + return localStorage.getItem(STORAGE_KEY_MONACO_ENABLED) === "true"; } catch { return false; } } +// Get saved side-by-side preference (default: true for desktop) +function getSideBySidePreference(): boolean { + try { + const stored = localStorage.getItem(STORAGE_KEY_SIDE_BY_SIDE); + if (stored !== null) { + return stored === "true"; + } + // Default to side-by-side on desktop, inline on mobile + return window.innerWidth >= 768; + } catch { + return window.innerWidth >= 768; + } +} + +function setSideBySidePreference(value: boolean): void { + try { + localStorage.setItem(STORAGE_KEY_SIDE_BY_SIDE, value ? "true" : "false"); + } catch { + // Ignore storage errors + } +} + // Display data structure from the patch tool interface PatchDisplayData { path: string; @@ -72,13 +98,7 @@ function loadMonaco(): Promise { } // Simple diff view component (default) -function SimpleDiffView({ - displayData, - executionTime, -}: { - displayData: PatchDisplayData | null; - executionTime?: string; -}) { +function SimpleDiffView({ displayData }: { displayData: PatchDisplayData | null }) { // Get diff text from displayData or fall back to empty const diff = displayData?.diff || ""; @@ -86,35 +106,27 @@ function SimpleDiffView({ const lines = diff ? diff.split("\n") : []; return ( -
- {executionTime && ( -
- Diff: - {executionTime} -
- )} - 
-        {lines.map((line, idx) => {
-          // Determine line type for styling
-          let className = "patch-diff-line";
-          if (line.startsWith("+") && !line.startsWith("+++")) {
-            className += " patch-diff-addition";
-          } else if (line.startsWith("-") && !line.startsWith("---")) {
-            className += " patch-diff-deletion";
-          } else if (line.startsWith("@@")) {
-            className += " patch-diff-hunk";
-          } else if (line.startsWith("---") || line.startsWith("+++")) {
-            className += " patch-diff-header";
-          }
+    
+      {lines.map((line, idx) => {
+        // Determine line type for styling
+        let className = "patch-diff-line";
+        if (line.startsWith("+") && !line.startsWith("+++")) {
+          className += " patch-diff-addition";
+        } else if (line.startsWith("-") && !line.startsWith("---")) {
+          className += " patch-diff-deletion";
+        } else if (line.startsWith("@@")) {
+          className += " patch-diff-hunk";
+        } else if (line.startsWith("---") || line.startsWith("+++")) {
+          className += " patch-diff-header";
+        }
 
-          return (
-            

-              {line || " "}
-            

-          );
-        })}
-      

-
+ return ( +
+ {line || " "} +
+ ); + })} + ); } @@ -122,26 +134,32 @@ function SimpleDiffView({ function MonacoDiffView({ displayData, isMobile, + sideBySide, onCommentTextChange, filename, }: { displayData: PatchDisplayData; isMobile: boolean; + sideBySide: boolean; onCommentTextChange?: (text: string) => void; filename: string; }) { const [monacoLoaded, setMonacoLoaded] = useState(false); + const [isVisible, setIsVisible] = useState(false); + const [editorHeight, setEditorHeight] = useState(100); const [showCommentDialog, setShowCommentDialog] = useState<{ line: number; selectedText?: string; } | null>(null); const [commentText, setCommentText] = useState(""); + const containerRef = useRef(null); const editorContainerRef = useRef(null); const editorRef = useRef(null); const monacoRef = useRef(null); const commentInputRef = useRef(null); const hoverDecorationsRef = useRef([]); + const heightSetRef = useRef(false); const modelsRef = useRef<{ original: Monaco.editor.ITextModel | null; modified: Monaco.editor.ITextModel | null; @@ -150,23 +168,58 @@ function MonacoDiffView({ modified: null, }); - // Load Monaco + // Intersection observer for lazy loading useEffect(() => { - if (!monacoLoaded) { - loadMonaco() - .then((monaco) => { - monacoRef.current = monaco; - setMonacoLoaded(true); - }) - .catch((err) => { - console.error("Failed to load Monaco:", err); - }); + const container = containerRef.current; + if (!container) return; + + const observer = new IntersectionObserver( + (entries) => { + for (const entry of entries) { + if (entry.isIntersecting) { + setIsVisible(true); + // Once visible, we don't need to observe anymore + observer.disconnect(); + } + } + }, + { + rootMargin: "100px", // Start loading a bit before it's visible + threshold: 0, + }, + ); + + observer.observe(container); + + return () => observer.disconnect(); + }, []); + + // Load Monaco only when visible + useEffect(() => { + if (!isVisible || monacoLoaded) return; + + loadMonaco() + .then((monaco) => { + monacoRef.current = monaco; + setMonacoLoaded(true); + }) + .catch((err) => { + console.error("Failed to load Monaco:", err); + }); + }, [isVisible, monacoLoaded]); + + // Update side-by-side mode when prop changes + useEffect(() => { + if (editorRef.current) { + editorRef.current.updateOptions({ renderSideBySide: sideBySide }); + // Reset height flag to allow recalculation after mode change + heightSetRef.current = false; } - }, [monacoLoaded]); + }, [sideBySide]); - // Create Monaco editor when data is ready + // Create Monaco editor when data is ready and visible useEffect(() => { - if (!monacoLoaded || !editorContainerRef.current || !monacoRef.current) { + if (!monacoLoaded || !isVisible || !editorContainerRef.current || !monacoRef.current) { return; } @@ -186,6 +239,9 @@ function MonacoDiffView({ modelsRef.current.modified = null; } + // Reset height tracking for new editor + heightSetRef.current = false; + // Get language from file extension const ext = "." + (displayData.path.split(".").pop()?.toLowerCase() || ""); const languages = monaco.languages.getLanguages(); @@ -212,13 +268,13 @@ function MonacoDiffView({ const modifiedModel = monaco.editor.createModel(displayData.newContent, language, modifiedUri); modelsRef.current = { original: originalModel, modified: modifiedModel }; - // Create diff editor + // Create diff editor with collapsed unchanged regions const diffEditor = monaco.editor.createDiffEditor(editorContainerRef.current, { theme: isDarkModeActive() ? "vs-dark" : "vs", readOnly: true, originalEditable: false, automaticLayout: true, - renderSideBySide: !isMobile, + renderSideBySide: sideBySide, enableSplitViewResizing: true, renderIndicators: true, renderMarginRevertIcon: false, @@ -236,6 +292,19 @@ function MonacoDiffView({ contextmenu: false, links: false, folding: !isMobile, + // Hide unchanged regions to show only edited sections + hideUnchangedRegions: { + enabled: true, + revealLineCount: 2, // Show 2 lines of context around changes + minimumLineCount: 3, // Hide regions with 3+ unchanged lines + contextLineCount: 2, // Context lines to show when expanding + }, + // Disable scrollbar when content fits + scrollbar: { + vertical: "auto", + horizontal: "auto", + alwaysConsumeMouseWheel: false, + }, }); diffEditor.setModel({ @@ -245,10 +314,33 @@ function MonacoDiffView({ editorRef.current = diffEditor; - // Add click handler for commenting if callback is provided - if (onCommentTextChange) { + // Function to update height - only do this once to avoid scroll disruption + const updateHeight = () => { + if (heightSetRef.current) return; + const modifiedEditor = diffEditor.getModifiedEditor(); + const contentHeight = modifiedEditor.getContentHeight(); + + if (contentHeight > 0) { + // Add small buffer, no max height - let it expand fully + const newHeight = Math.max(60, contentHeight + 4); + heightSetRef.current = true; + setEditorHeight(newHeight); + } + }; + + // Update height after diff is computed + // Monaco needs time to compute the diff and layout + const heightUpdateTimer = setTimeout(updateHeight, 200); + // Also listen for content size change (fires when diff is computed) + const modifiedEditor = diffEditor.getModifiedEditor(); + const contentSizeDisposable = modifiedEditor.onDidContentSizeChange(() => { + updateHeight(); + }); + + // Add click handler for commenting if callback is provided + if (onCommentTextChange) { const openCommentDialog = (lineNumber: number) => { const model = modifiedEditor.getModel(); const selection = modifiedEditor.getSelection(); @@ -323,6 +415,8 @@ function MonacoDiffView({ // Cleanup function return () => { + clearTimeout(heightUpdateTimer); + contentSizeDisposable.dispose(); if (editorRef.current) { editorRef.current.dispose(); editorRef.current = null; @@ -336,7 +430,7 @@ function MonacoDiffView({ modelsRef.current.modified = null; } }; - }, [monacoLoaded, displayData, isMobile, onCommentTextChange]); + }, [monacoLoaded, isVisible, displayData, isMobile, onCommentTextChange, sideBySide]); // Update Monaco theme when dark mode changes useEffect(() => { @@ -385,24 +479,25 @@ function MonacoDiffView({ setCommentText(""); }, [showCommentDialog, commentText, onCommentTextChange, filename]); - // Calculate editor height based on content - const getEditorHeight = () => { - const lineCount = Math.max( - displayData.oldContent.split("\n").length, - displayData.newContent.split("\n").length, - ); - // Clamp between 100px and 400px, with 18px per line - const height = Math.min(400, Math.max(100, lineCount * 18 + 20)); - return `${height}px`; - }; - return ( - <> -
+
+ {/* Monaco editor container */} + {!isVisible ? ( +
+ Scroll to load diff... +
+ ) : !monacoLoaded ? ( +
+
+ Loading editor... +
+ ) : ( +
+ )} {/* Comment dialog */} {showCommentDialog && onCommentTextChange && ( @@ -443,7 +538,71 @@ function MonacoDiffView({
)} - +
+ ); +} + +// Side-by-side toggle icon component +function DiffModeToggle({ sideBySide, onToggle }: { sideBySide: boolean; onToggle: () => void }) { + return ( + ); } @@ -452,13 +611,13 @@ function PatchTool({ isRunning, toolResult, hasError, - executionTime, display, onCommentTextChange, }: PatchToolProps) { // Default to collapsed for errors (since agents typically recover), expanded otherwise const [isExpanded, setIsExpanded] = useState(!hasError); const [isMobile, setIsMobile] = useState(window.innerWidth < 768); + const [sideBySide, setSideBySide] = useState(() => !isMobile && getSideBySidePreference()); // Check feature flag for Monaco diff view const useMonaco = useMonacoDiff(); @@ -472,6 +631,13 @@ function PatchTool({ return () => window.removeEventListener("resize", handleResize); }, []); + // Toggle side-by-side mode + const toggleSideBySide = useCallback(() => { + const newValue = !sideBySide; + setSideBySide(newValue); + setSideBySidePreference(newValue); + }, [sideBySide]); + // Extract path from toolInput const path = typeof toolInput === "object" && @@ -502,6 +668,10 @@ function PatchTool({ // Extract filename from path or diff headers const filename = displayData?.path || path || "patch"; + // Show toggle only for Monaco view on desktop when expanded and complete + const showDiffToggle = + useMonaco && !isMobile && isExpanded && isComplete && !hasError && displayData; + return (
✗} {isComplete && !hasError && ✓}
- + + + + +
{isExpanded && (
{isComplete && !hasError && displayData && (
- {executionTime && ( -
- Diff: - {executionTime} -
- )} - {useMonaco ? ( ) : ( - + )}
)} {isComplete && hasError && (
-
- Error: - {executionTime && {executionTime}} - 
{errorMessage || "Patch failed"}
)} diff --git a/ui/src/styles.css b/ui/src/styles.css index 5f416a2edb36ad35973450d48d885958ed00be4d..dacffa8cbdcb1418f14f66b3fb3b887ae3899d53 100644 --- a/ui/src/styles.css +++ b/ui/src/styles.css @@ -1188,6 +1188,13 @@ button { flex-shrink: 0; } +.patch-tool-header-controls { + display: flex; + align-items: center; + gap: 0.25rem; + flex-shrink: 0; +} + .patch-tool-toggle { background: none; border: none; @@ -1204,6 +1211,26 @@ button { color: var(--text-primary); } +.patch-tool-diff-mode-toggle { + background: none; + border: 1px solid var(--border); + border-radius: 0.25rem; + cursor: pointer; + padding: 0.25rem; + display: flex; + align-items: center; + justify-content: center; + color: var(--text-secondary); + flex-shrink: 0; + transition: all 0.15s ease; +} + +.patch-tool-diff-mode-toggle:hover { + color: var(--text-primary); + background: var(--bg-secondary); + border-color: var(--text-tertiary); +} + .patch-tool-details { padding: 0 1rem 0.75rem 1rem; display: flex; @@ -1295,6 +1322,41 @@ button { font-style: italic; } +/* Patch Tool Monaco Container */ +.patch-tool-monaco-container { + display: flex; + flex-direction: column; + gap: 0.25rem; +} + +/* Patch Tool Monaco Placeholder (for lazy loading) */ +.patch-tool-monaco-placeholder { + display: flex; + align-items: center; + justify-content: center; + gap: 0.5rem; + background: var(--bg-tertiary); + border: 1px solid var(--border); + border-radius: 0.25rem; + color: var(--text-secondary); + font-size: 0.75rem; +} + +.spinner-small { + width: 14px; + height: 14px; + border: 2px solid var(--border); + border-top-color: var(--primary); + border-radius: 50%; + animation: spin 0.8s linear infinite; +} + +@keyframes spin { + to { + transform: rotate(360deg); + } +} + /* Patch Tool Monaco Editor */ .patch-tool-monaco-editor { border: 1px solid var(--border);