From e3bb6adcba2dfd09739dfdc047cdf8903b31576a Mon Sep 17 00:00:00 2001 From: Philip Zeyliger Date: Sun, 11 Jan 2026 15:31:01 +0000 Subject: [PATCH] shelley/ui: fix Monaco model leak causing Firefox crash 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 --- loop/predictable.go | 45 +++++++++++++++++++++++++++++++ ui/e2e/tool-components.spec.ts | 47 +++++++++++++++++++++++++++++++++ ui/src/components/PatchTool.tsx | 32 +++++++++++++++++++++- 3 files changed, 123 insertions(+), 1 deletion(-) diff --git a/loop/predictable.go b/loop/predictable.go index b93c2cf23496f3390cceee73e2d6cd5baf7c362b..8dcf8401859489a1ecdcfb0113b58aa5c9577eae 100644 --- a/loop/predictable.go +++ b/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 { diff --git a/ui/e2e/tool-components.spec.ts b/ui/e2e/tool-components.spec.ts index 5a0fd2d50c29eb8eb780557520c856439817bee1..1f67d1786144eee0286a1c8aebc6fa640d046c88 100644 --- a/ui/e2e/tool-components.spec.ts +++ b/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'); diff --git a/ui/src/components/PatchTool.tsx b/ui/src/components/PatchTool.tsx index 52e5682afa3c623ce1dbca657aa7e81d67bca8b1..3db864435103b30cd50680c90a6b1f511a0af241 100644 --- a/ui/src/components/PatchTool.tsx +++ b/ui/src/components/PatchTool.tsx @@ -86,6 +86,13 @@ function PatchTool({ const monacoRef = useRef(null); const commentInputRef = useRef(null); const hoverDecorationsRef = useRef([]); + 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]);