shelley/ui: default to simple diff view, feature flag for Monaco

Philip Zeyliger created

Prompt: In a sort of recent commit, we switched the patch tool output to
use Monaco diffs. These seem to not be working well yet. Let's reintroduce
the old simple diff view. Keep the Monaco code behind a feature flag that
can be set with local storage so I can keep trying the new thing.

Use localStorage.setItem('shelley-use-monaco-diff', 'true') to enable
the Monaco diff view. The simple text-based diff view is now the default
as the Monaco diff had some issues.

Both views are still available:
- Simple diff: default, shows unified diff with syntax highlighting
- Monaco diff: feature-flagged, side-by-side view with commenting

Change summary

ui/src/components/PatchTool.tsx | 297 ++++++++++++++++++++++------------
ui/src/styles.css               |  49 +++++
2 files changed, 239 insertions(+), 107 deletions(-)

Detailed changes

ui/src/components/PatchTool.tsx 🔗

@@ -3,6 +3,15 @@ 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)
+function useMonacoDiff(): boolean {
+  try {
+    return localStorage.getItem("shelley-use-monaco-diff") === "true";
+  } catch {
+    return false;
+  }
+}
+
 // Display data structure from the patch tool
 interface PatchDisplayData {
   path: string;
@@ -62,19 +71,66 @@ function loadMonaco(): Promise<typeof Monaco> {
   return monacoLoadPromise;
 }
 
-function PatchTool({
-  toolInput,
-  isRunning,
-  toolResult,
-  hasError,
+// Simple diff view component (default)
+function SimpleDiffView({
+  displayData,
   executionTime,
-  display,
+}: {
+  displayData: PatchDisplayData | null;
+  executionTime?: string;
+}) {
+  // Get diff text from displayData or fall back to empty
+  const diff = displayData?.diff || "";
+
+  // Parse unified diff to extract lines
+  const lines = diff ? diff.split("\n") : [];
+
+  return (
+    <div className="patch-tool-section">
+      {executionTime && (
+        <div className="patch-tool-label">
+          <span>Diff:</span>
+          <span className="patch-tool-time">{executionTime}</span>
+        </div>
+      )}
+      <pre className="patch-tool-diff">
+        {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 (
+            <div key={idx} className={className}>
+              {line || " "}
+            </div>
+          );
+        })}
+      </pre>
+    </div>
+  );
+}
+
+// Monaco diff view component (feature-flagged)
+function MonacoDiffView({
+  displayData,
+  isMobile,
   onCommentTextChange,
-}: PatchToolProps) {
-  // Default to collapsed for errors (since agents typically recover), expanded otherwise
-  const [isExpanded, setIsExpanded] = useState(!hasError);
+  filename,
+}: {
+  displayData: PatchDisplayData;
+  isMobile: boolean;
+  onCommentTextChange?: (text: string) => void;
+  filename: string;
+}) {
   const [monacoLoaded, setMonacoLoaded] = useState(false);
-  const [isMobile, setIsMobile] = useState(window.innerWidth < 768);
   const [showCommentDialog, setShowCommentDialog] = useState<{
     line: number;
     selectedText?: string;
@@ -94,48 +150,9 @@ function PatchTool({
     modified: null,
   });
 
-  // Track viewport size
+  // Load Monaco
   useEffect(() => {
-    const handleResize = () => {
-      setIsMobile(window.innerWidth < 768);
-    };
-    window.addEventListener("resize", handleResize);
-    return () => window.removeEventListener("resize", handleResize);
-  }, []);
-
-  // Extract path from toolInput
-  const path =
-    typeof toolInput === "object" &&
-    toolInput !== null &&
-    "path" in toolInput &&
-    typeof toolInput.path === "string"
-      ? toolInput.path
-      : typeof toolInput === "string"
-        ? toolInput
-        : "";
-
-  // Parse display data (structured format from backend)
-  const displayData: PatchDisplayData | null =
-    display &&
-    typeof display === "object" &&
-    "path" in display &&
-    "oldContent" in display &&
-    "newContent" in display
-      ? (display as PatchDisplayData)
-      : null;
-
-  // Extract error message from toolResult if present
-  const errorMessage =
-    toolResult && toolResult.length > 0 && toolResult[0].Text ? toolResult[0].Text : "";
-
-  const isComplete = !isRunning && toolResult !== undefined;
-
-  // Extract filename from path or diff headers
-  const filename = displayData?.path || path || "patch";
-
-  // Load Monaco when expanded and we have display data
-  useEffect(() => {
-    if (isExpanded && displayData && !monacoLoaded) {
+    if (!monacoLoaded) {
       loadMonaco()
         .then((monaco) => {
           monacoRef.current = monaco;
@@ -145,17 +162,11 @@ function PatchTool({
           console.error("Failed to load Monaco:", err);
         });
     }
-  }, [isExpanded, displayData, monacoLoaded]);
+  }, [monacoLoaded]);
 
   // Create Monaco editor when data is ready
   useEffect(() => {
-    if (
-      !monacoLoaded ||
-      !displayData ||
-      !editorContainerRef.current ||
-      !monacoRef.current ||
-      !isExpanded
-    ) {
+    if (!monacoLoaded || !editorContainerRef.current || !monacoRef.current) {
       return;
     }
 
@@ -325,7 +336,7 @@ function PatchTool({
         modelsRef.current.modified = null;
       }
     };
-  }, [monacoLoaded, displayData, isMobile, isExpanded, onCommentTextChange]);
+  }, [monacoLoaded, displayData, isMobile, onCommentTextChange]);
 
   // Update Monaco theme when dark mode changes
   useEffect(() => {
@@ -376,7 +387,6 @@ function PatchTool({
 
   // Calculate editor height based on content
   const getEditorHeight = () => {
-    if (!displayData) return "200px";
     const lineCount = Math.max(
       displayData.oldContent.split("\n").length,
       displayData.newContent.split("\n").length,
@@ -386,6 +396,112 @@ function PatchTool({
     return `${height}px`;
   };
 
+  return (
+    <>
+      <div
+        ref={editorContainerRef}
+        className="patch-tool-monaco-editor"
+        style={{ height: getEditorHeight(), width: "100%" }}
+      />
+
+      {/* Comment dialog */}
+      {showCommentDialog && onCommentTextChange && (
+        <div className="patch-tool-comment-dialog">
+          <h4>Add Comment (Line {showCommentDialog.line})</h4>
+          {showCommentDialog.selectedText && (
+            <pre className="patch-tool-selected-text">{showCommentDialog.selectedText}</pre>
+          )}
+          <textarea
+            ref={commentInputRef}
+            value={commentText}
+            onChange={(e) => setCommentText(e.target.value)}
+            placeholder="Enter your comment..."
+            className="patch-tool-comment-input"
+            autoFocus
+            onKeyDown={(e) => {
+              if (e.key === "Escape") {
+                setShowCommentDialog(null);
+              } else if (e.key === "Enter" && (e.ctrlKey || e.metaKey)) {
+                handleAddComment();
+              }
+            }}
+          />
+          <div className="patch-tool-comment-actions">
+            <button
+              onClick={() => setShowCommentDialog(null)}
+              className="patch-tool-btn patch-tool-btn-secondary"
+            >
+              Cancel
+            </button>
+            <button
+              onClick={handleAddComment}
+              className="patch-tool-btn patch-tool-btn-primary"
+              disabled={!commentText.trim()}
+            >
+              Add Comment
+            </button>
+          </div>
+        </div>
+      )}
+    </>
+  );
+}
+
+function PatchTool({
+  toolInput,
+  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);
+
+  // Check feature flag for Monaco diff view
+  const useMonaco = useMonacoDiff();
+
+  // Track viewport size
+  useEffect(() => {
+    const handleResize = () => {
+      setIsMobile(window.innerWidth < 768);
+    };
+    window.addEventListener("resize", handleResize);
+    return () => window.removeEventListener("resize", handleResize);
+  }, []);
+
+  // Extract path from toolInput
+  const path =
+    typeof toolInput === "object" &&
+    toolInput !== null &&
+    "path" in toolInput &&
+    typeof toolInput.path === "string"
+      ? toolInput.path
+      : typeof toolInput === "string"
+        ? toolInput
+        : "";
+
+  // Parse display data (structured format from backend)
+  const displayData: PatchDisplayData | null =
+    display &&
+    typeof display === "object" &&
+    "path" in display &&
+    "oldContent" in display &&
+    "newContent" in display
+      ? (display as PatchDisplayData)
+      : null;
+
+  // Extract error message from toolResult if present
+  const errorMessage =
+    toolResult && toolResult.length > 0 && toolResult[0].Text ? toolResult[0].Text : "";
+
+  const isComplete = !isRunning && toolResult !== undefined;
+
+  // Extract filename from path or diff headers
+  const filename = displayData?.path || path || "patch";
+
   return (
     <div
       className="patch-tool"
@@ -436,12 +552,19 @@ function PatchTool({
                 </div>
               )}
 
-              {/* Monaco diff editor */}
-              <div
-                ref={editorContainerRef}
-                className="patch-tool-monaco-editor"
-                style={{ height: getEditorHeight(), width: "100%" }}
-              />
+              {useMonaco ? (
+                <MonacoDiffView
+                  displayData={displayData}
+                  isMobile={isMobile}
+                  onCommentTextChange={onCommentTextChange}
+                  filename={filename}
+                />
+              ) : (
+                <SimpleDiffView
+                  displayData={displayData}
+                  executionTime={undefined} // Already shown above
+                />
+              )}
             </div>
           )}
 
@@ -462,46 +585,6 @@ function PatchTool({
           )}
         </div>
       )}
-
-      {/* Comment dialog */}
-      {showCommentDialog && onCommentTextChange && (
-        <div className="patch-tool-comment-dialog">
-          <h4>Add Comment (Line {showCommentDialog.line})</h4>
-          {showCommentDialog.selectedText && (
-            <pre className="patch-tool-selected-text">{showCommentDialog.selectedText}</pre>
-          )}
-          <textarea
-            ref={commentInputRef}
-            value={commentText}
-            onChange={(e) => setCommentText(e.target.value)}
-            placeholder="Enter your comment..."
-            className="patch-tool-comment-input"
-            autoFocus
-            onKeyDown={(e) => {
-              if (e.key === "Escape") {
-                setShowCommentDialog(null);
-              } else if (e.key === "Enter" && (e.ctrlKey || e.metaKey)) {
-                handleAddComment();
-              }
-            }}
-          />
-          <div className="patch-tool-comment-actions">
-            <button
-              onClick={() => setShowCommentDialog(null)}
-              className="patch-tool-btn patch-tool-btn-secondary"
-            >
-              Cancel
-            </button>
-            <button
-              onClick={handleAddComment}
-              className="patch-tool-btn patch-tool-btn-primary"
-              disabled={!commentText.trim()}
-            >
-              Add Comment
-            </button>
-          </div>
-        </div>
-      )}
     </div>
   );
 }

ui/src/styles.css 🔗

@@ -1231,6 +1231,55 @@ button {
   color: var(--text-secondary);
 }
 
+/* Patch Tool Simple Diff View */
+.patch-tool-diff {
+  font-family: var(--font-mono);
+  font-size: 0.875rem;
+  background: var(--bg-base);
+  border: 1px solid var(--border);
+  border-radius: 0.25rem;
+  padding: 0.75rem;
+  margin: 0;
+  overflow-x: auto;
+  line-height: 1.4;
+}
+
+.patch-diff-line {
+  white-space: pre;
+  display: block;
+}
+
+.patch-diff-addition {
+  background: rgba(34, 197, 94, 0.1);
+  color: #16a34a;
+}
+
+.dark .patch-diff-addition {
+  background: rgba(34, 197, 94, 0.15);
+  color: #86efac;
+}
+
+.patch-diff-deletion {
+  background: rgba(239, 68, 68, 0.1);
+  color: #dc2626;
+}
+
+.dark .patch-diff-deletion {
+  background: rgba(239, 68, 68, 0.15);
+  color: #fca5a5;
+}
+
+.patch-diff-hunk {
+  color: var(--text-secondary);
+  background: var(--bg-tertiary);
+  font-weight: 500;
+}
+
+.patch-diff-header {
+  color: var(--text-tertiary);
+  font-style: italic;
+}
+
 /* Patch Tool Monaco Editor */
 .patch-tool-monaco-editor {
   border: 1px solid var(--border);