shelley/ui: fix Monaco model leak causing Firefox crash

Philip Zeyliger created

Prompt: fix and add a test for the following new Shelley inline diff view causes the page to break in Firefox; it goes completely blank and then I get this error:
Error: ModelService: Cannot add model because it already exists!

The inline diff view in PatchTool was crashing Firefox with 'Cannot add
model because it already exists!' when collapsing and expanding the diff.

Root cause: Monaco editor models were created but never disposed during
cleanup. When the component unmounted and remounted (or the useEffect
re-ran), new models were created with potentially conflicting URIs.

Fix:
- Track Monaco models in a ref (modelsRef)
- Dispose models in the cleanup function alongside the editor
- Defensively check for and dispose existing models before creating new
  ones (in case of timing issues)

Also added:
- 'patch success' command to predictable model (uses overwrite operation)
- E2E test that verifies collapse/expand cycles don't cause errors

Change summary

loop/predictable.go             | 45 +++++++++++++++++++++++++++++++++
ui/e2e/tool-components.spec.ts  | 47 +++++++++++++++++++++++++++++++++++
ui/src/components/PatchTool.tsx | 32 +++++++++++++++++++++++
3 files changed, 123 insertions(+), 1 deletion(-)

Detailed changes

loop/predictable.go 🔗

@@ -120,6 +120,10 @@ func (s *PredictableService) Do(ctx context.Context, req *llm.Request) (*llm.Res
 		// Trigger a patch that will fail (file doesn't exist)
 		return s.makePatchToolResponse("/nonexistent/file/that/does/not/exist.txt", inputTokens), nil
 
+	case "patch success":
+		// Trigger a patch that will succeed (using overwrite, which creates the file)
+		return s.makePatchToolResponseOverwrite("/tmp/test-patch-success.txt", inputTokens), nil
+
 	case "patch bad json":
 		// Trigger a patch with malformed JSON (simulates Anthropic sending invalid JSON)
 		return s.makeMalformedPatchToolResponse(inputTokens), nil
@@ -336,6 +340,47 @@ func (s *PredictableService) makePatchToolResponse(filePath string, inputTokens
 	}
 }
 
+// makePatchToolResponseOverwrite creates a response that uses overwrite operation (always succeeds)
+func (s *PredictableService) makePatchToolResponseOverwrite(filePath string, inputTokens uint64) *llm.Response {
+	toolInputData := map[string]interface{}{
+		"path": filePath,
+		"patches": []map[string]string{
+			{
+				"operation": "overwrite",
+				"newText":   "This is the new content of the file.\nLine 2\nLine 3\n",
+			},
+		},
+	}
+	toolInputBytes, _ := json.Marshal(toolInputData)
+	toolInput := json.RawMessage(toolInputBytes)
+	responseText := fmt.Sprintf("I'll create/overwrite the file: %s", filePath)
+	outputTokens := uint64(len(responseText)/4 + len(toolInputBytes)/4)
+	if outputTokens == 0 {
+		outputTokens = 1
+	}
+	return &llm.Response{
+		ID:    fmt.Sprintf("pred-patch-overwrite-%d", time.Now().UnixNano()),
+		Type:  "message",
+		Role:  llm.MessageRoleAssistant,
+		Model: "predictable-v1",
+		Content: []llm.Content{
+			{Type: llm.ContentTypeText, Text: responseText},
+			{
+				ID:        fmt.Sprintf("tool_%d", time.Now().UnixNano()%1000),
+				Type:      llm.ContentTypeToolUse,
+				ToolName:  "patch",
+				ToolInput: toolInput,
+			},
+		},
+		StopReason: llm.StopReasonToolUse,
+		Usage: llm.Usage{
+			InputTokens:  inputTokens,
+			OutputTokens: outputTokens,
+			CostUSD:      0.0,
+		},
+	}
+}
+
 // makeMalformedPatchToolResponse creates a response with malformed JSON that will fail to parse
 // This simulates when Anthropic sends back invalid JSON in the tool input
 func (s *PredictableService) makeMalformedPatchToolResponse(inputTokens uint64) *llm.Response {

ui/e2e/tool-components.spec.ts 🔗

@@ -145,6 +145,53 @@ test.describe('Tool Component Verification', () => {
     await expect(navigateTool.locator('.tool-command').filter({ hasText: 'https://example.com' })).toBeVisible();
   });
 
+  test('patch tool can be collapsed and expanded without errors', async ({ page }) => {
+    await page.goto('/');
+    await page.waitForLoadState('domcontentloaded');
+
+    const messageInput = page.getByTestId('message-input');
+    const sendButton = page.getByTestId('send-button');
+
+    // Trigger a successful patch tool (uses overwrite operation which always succeeds)
+    await messageInput.fill('patch success');
+    await sendButton.click();
+
+    // Wait for successful patch tool with Monaco editor
+    // Use specific locator to find the successful patch (not the failed ones from other tests)
+    const patchTool = page.locator('.patch-tool[data-testid="tool-call-completed"]').filter({ hasText: 'test-patch-success.txt' }).first();
+    await expect(patchTool).toBeVisible({ timeout: 30000 });
+    // Wait for Monaco editor to be fully rendered (only visible for successful patches)
+    await expect(patchTool.locator('.patch-tool-monaco-editor')).toBeVisible({ timeout: 10000 });
+
+    // Get console errors before toggling
+    const errors: string[] = [];
+    page.on('pageerror', (error) => errors.push(error.message));
+
+    const header = patchTool.locator('.patch-tool-header');
+
+    // Collapse
+    await header.click();
+    await expect(patchTool.locator('.patch-tool-details')).toBeHidden();
+
+    // Expand - Monaco should reinitialize
+    await header.click();
+    await expect(patchTool.locator('.patch-tool-details')).toBeVisible();
+    await expect(patchTool.locator('.patch-tool-monaco-editor')).toBeVisible({ timeout: 10000 });
+
+    // Collapse again
+    await header.click();
+    await expect(patchTool.locator('.patch-tool-details')).toBeHidden();
+
+    // Expand again - this was triggering "Cannot add model because it already exists!" in Firefox
+    await header.click();
+    await expect(patchTool.locator('.patch-tool-details')).toBeVisible();
+    await expect(patchTool.locator('.patch-tool-monaco-editor')).toBeVisible({ timeout: 10000 });
+
+    // Check no Monaco model errors occurred
+    const modelErrors = errors.filter(e => e.includes('model') && e.includes('already exists'));
+    expect(modelErrors).toHaveLength(0);
+  });
+
   test('emoji sizes are consistent across all tools', async ({ page }) => {
     await page.goto('/');
     await page.waitForLoadState('domcontentloaded');

ui/src/components/PatchTool.tsx 🔗

@@ -86,6 +86,13 @@ function PatchTool({
   const monacoRef = useRef<typeof Monaco | null>(null);
   const commentInputRef = useRef<HTMLTextAreaElement>(null);
   const hoverDecorationsRef = useRef<string[]>([]);
+  const modelsRef = useRef<{
+    original: Monaco.editor.ITextModel | null;
+    modified: Monaco.editor.ITextModel | null;
+  }>({
+    original: null,
+    modified: null,
+  });
 
   // Track viewport size
   useEffect(() => {
@@ -154,11 +161,19 @@ function PatchTool({
 
     const monaco = monacoRef.current;
 
-    // Dispose previous editor
+    // Dispose previous editor and models
     if (editorRef.current) {
       editorRef.current.dispose();
       editorRef.current = null;
     }
+    if (modelsRef.current.original) {
+      modelsRef.current.original.dispose();
+      modelsRef.current.original = null;
+    }
+    if (modelsRef.current.modified) {
+      modelsRef.current.modified.dispose();
+      modelsRef.current.modified = null;
+    }
 
     // Get language from file extension
     const ext = "." + (displayData.path.split(".").pop()?.toLowerCase() || "");
@@ -176,8 +191,15 @@ function PatchTool({
     const originalUri = monaco.Uri.file(`patch-original-${timestamp}-${displayData.path}`);
     const modifiedUri = monaco.Uri.file(`patch-modified-${timestamp}-${displayData.path}`);
 
+    // Check for and dispose any existing models with these URIs (defensive, shouldn't happen)
+    const existingOriginal = monaco.editor.getModel(originalUri);
+    if (existingOriginal) existingOriginal.dispose();
+    const existingModified = monaco.editor.getModel(modifiedUri);
+    if (existingModified) existingModified.dispose();
+
     const originalModel = monaco.editor.createModel(displayData.oldContent, language, originalUri);
     const modifiedModel = monaco.editor.createModel(displayData.newContent, language, modifiedUri);
+    modelsRef.current = { original: originalModel, modified: modifiedModel };
 
     // Create diff editor
     const diffEditor = monaco.editor.createDiffEditor(editorContainerRef.current, {
@@ -294,6 +316,14 @@ function PatchTool({
         editorRef.current.dispose();
         editorRef.current = null;
       }
+      if (modelsRef.current.original) {
+        modelsRef.current.original.dispose();
+        modelsRef.current.original = null;
+      }
+      if (modelsRef.current.modified) {
+        modelsRef.current.modified.dispose();
+        modelsRef.current.modified = null;
+      }
     };
   }, [monacoLoaded, displayData, isMobile, isExpanded, onCommentTextChange]);