Skip to content

Commit 9bfcabb

Browse files
mrubensmrubens
committed
Better error handling when installing marketplace items
1 parent 597722f commit 9bfcabb

File tree

6 files changed

+312
-10
lines changed

6 files changed

+312
-10
lines changed

‎src/core/webview/webviewMessageHandler.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1498,8 +1498,21 @@ export const webviewMessageHandler = async (
14981498
)
14991499
await provider.postStateToWebview()
15001500
console.log(`Marketplace item installed and config file opened: ${configFilePath}`)
1501+
// Send success message to webview
1502+
provider.postMessageToWebview({
1503+
type: "marketplaceInstallResult",
1504+
success: true,
1505+
slug: message.mpItem.id,
1506+
})
15011507
} catch (error) {
15021508
console.error(`Error installing marketplace item: ${error}`)
1509+
// Send error message to webview
1510+
provider.postMessageToWebview({
1511+
type: "marketplaceInstallResult",
1512+
success: false,
1513+
error: error instanceof Error ? error.message : String(error),
1514+
slug: message.mpItem.id,
1515+
})
15031516
}
15041517
}
15051518
break

‎src/services/marketplace/SimpleInstaller.ts‎

Lines changed: 48 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,21 @@ export class SimpleInstaller {
4848
try {
4949
const existing = await fs.readFile(filePath, "utf-8")
5050
existingData = yaml.parse(existing) || { customModes: [] }
51-
} catch (error) {
52-
// File doesn't exist, use default structure
51+
} catch (error: any) {
52+
if (error.code === "ENOENT") {
53+
// File doesn't exist, use default structure - this is fine
54+
existingData = { customModes: [] }
55+
} else if (error.name === "YAMLParseError" || error.message?.includes("YAML")) {
56+
// YAML parsing error - don't overwrite the file!
57+
const fileName = target === "project" ? ".roomodes" : "custom-modes.yaml"
58+
throw new Error(
59+
`Cannot install mode: The ${fileName} file contains invalid YAML. ` +
60+
`Please fix the syntax errors in the file before installing new modes.`,
61+
)
62+
} else {
63+
// Other unexpected errors - re-throw
64+
throw error
65+
}
5366
}
5467

5568
// Ensure customModes array exists
@@ -168,8 +181,21 @@ export class SimpleInstaller {
168181
try {
169182
const existing = await fs.readFile(filePath, "utf-8")
170183
existingData = JSON.parse(existing) || { mcpServers: {} }
171-
} catch (error) {
172-
// File doesn't exist, use default structure
184+
} catch (error: any) {
185+
if (error.code === "ENOENT") {
186+
// File doesn't exist, use default structure
187+
existingData = { mcpServers: {} }
188+
} else if (error instanceof SyntaxError) {
189+
// JSON parsing error - don't overwrite the file!
190+
const fileName = target === "project" ? ".roo/mcp.json" : "mcp-settings.json"
191+
throw new Error(
192+
`Cannot install MCP server: The ${fileName} file contains invalid JSON. ` +
193+
`Please fix the syntax errors in the file before installing new servers.`,
194+
)
195+
} else {
196+
// Other unexpected errors - re-throw
197+
throw error
198+
}
173199
}
174200

175201
// Ensure mcpServers object exists
@@ -222,7 +248,18 @@ export class SimpleInstaller {
222248

223249
try {
224250
const existing = await fs.readFile(filePath, "utf-8")
225-
const existingData = yaml.parse(existing)
251+
let existingData: any
252+
253+
try {
254+
existingData = yaml.parse(existing)
255+
} catch (parseError) {
256+
// If we can't parse the file, we can't safely remove a mode
257+
const fileName = target === "project" ? ".roomodes" : "custom-modes.yaml"
258+
throw new Error(
259+
`Cannot remove mode: The ${fileName} file contains invalid YAML. ` +
260+
`Please fix the syntax errors before removing modes.`,
261+
)
262+
}
226263

227264
if (existingData?.customModes) {
228265
// Parse the item content to get the slug
@@ -245,8 +282,12 @@ export class SimpleInstaller {
245282
// Always write back the file, even if empty
246283
await fs.writeFile(filePath, yaml.stringify(existingData), "utf-8")
247284
}
248-
} catch (error) {
249-
// File doesn't exist or other error, nothing to remove
285+
} catch (error: any) {
286+
if (error.code === "ENOENT") {
287+
// File doesn't exist, nothing to remove
288+
return
289+
}
290+
throw error
250291
}
251292
}
252293

‎src/services/marketplace/__tests__/SimpleInstaller.test.ts‎

Lines changed: 225 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,225 @@
1+
import { SimpleInstaller } from "../SimpleInstaller"
2+
import * as fs from "fs/promises"
3+
import * as yaml from "yaml"
4+
import * as vscode from "vscode"
5+
import { MarketplaceItem } from "../types"
6+
import * as path from "path"
7+
8+
jest.mock("fs/promises")
9+
jest.mock("vscode", () => ({
10+
workspace: {
11+
workspaceFolders: [
12+
{
13+
uri: { fsPath: "/test/workspace" },
14+
name: "test",
15+
index: 0,
16+
},
17+
],
18+
},
19+
}))
20+
jest.mock("../../../utils/globalContext")
21+
22+
const mockFs = fs as jest.Mocked<typeof fs>
23+
24+
describe("SimpleInstaller", () => {
25+
let installer: SimpleInstaller
26+
let mockContext: vscode.ExtensionContext
27+
28+
beforeEach(() => {
29+
mockContext = {} as vscode.ExtensionContext
30+
installer = new SimpleInstaller(mockContext)
31+
jest.clearAllMocks()
32+
33+
// Mock mkdir to always succeed
34+
mockFs.mkdir.mockResolvedValue(undefined as any)
35+
})
36+
37+
describe("installMode", () => {
38+
const mockModeItem: MarketplaceItem = {
39+
id: "test-mode",
40+
name: "Test Mode",
41+
description: "A test mode for testing",
42+
type: "mode",
43+
content: yaml.stringify({
44+
slug: "test",
45+
name: "Test Mode",
46+
roleDefinition: "Test role",
47+
groups: ["read"],
48+
}),
49+
}
50+
51+
it("should install mode when .roomodes file does not exist", async () => {
52+
// Mock file not found error
53+
const notFoundError = new Error("File not found") as any
54+
notFoundError.code = "ENOENT"
55+
mockFs.readFile.mockRejectedValueOnce(notFoundError)
56+
mockFs.writeFile.mockResolvedValueOnce(undefined as any)
57+
58+
const result = await installer.installItem(mockModeItem, { target: "project" })
59+
60+
expect(result.filePath).toBe(path.join("/test/workspace", ".roomodes"))
61+
expect(mockFs.writeFile).toHaveBeenCalled()
62+
63+
// Verify the written content contains the new mode
64+
const writtenContent = mockFs.writeFile.mock.calls[0][1] as string
65+
const writtenData = yaml.parse(writtenContent)
66+
expect(writtenData.customModes).toHaveLength(1)
67+
expect(writtenData.customModes[0].slug).toBe("test")
68+
})
69+
70+
it("should install mode when .roomodes contains valid YAML", async () => {
71+
const existingContent = yaml.stringify({
72+
customModes: [{ slug: "existing", name: "Existing Mode", roleDefinition: "Existing", groups: [] }],
73+
})
74+
75+
mockFs.readFile.mockResolvedValueOnce(existingContent)
76+
mockFs.writeFile.mockResolvedValueOnce(undefined as any)
77+
78+
await installer.installItem(mockModeItem, { target: "project" })
79+
80+
expect(mockFs.writeFile).toHaveBeenCalled()
81+
const writtenContent = mockFs.writeFile.mock.calls[0][1] as string
82+
const writtenData = yaml.parse(writtenContent)
83+
84+
// Should contain both existing and new mode
85+
expect(writtenData.customModes).toHaveLength(2)
86+
expect(writtenData.customModes.find((m: any) => m.slug === "existing")).toBeDefined()
87+
expect(writtenData.customModes.find((m: any) => m.slug === "test")).toBeDefined()
88+
})
89+
90+
it("should throw error when .roomodes contains invalid YAML", async () => {
91+
const invalidYaml = "invalid: yaml: content: {"
92+
93+
mockFs.readFile.mockResolvedValueOnce(invalidYaml)
94+
95+
await expect(installer.installItem(mockModeItem, { target: "project" })).rejects.toThrow(
96+
"Cannot install mode: The .roomodes file contains invalid YAML",
97+
)
98+
99+
// Should NOT write to file
100+
expect(mockFs.writeFile).not.toHaveBeenCalled()
101+
})
102+
103+
it("should replace existing mode with same slug", async () => {
104+
const existingContent = yaml.stringify({
105+
customModes: [{ slug: "test", name: "Old Test Mode", roleDefinition: "Old role", groups: [] }],
106+
})
107+
108+
mockFs.readFile.mockResolvedValueOnce(existingContent)
109+
mockFs.writeFile.mockResolvedValueOnce(undefined as any)
110+
111+
await installer.installItem(mockModeItem, { target: "project" })
112+
113+
const writtenContent = mockFs.writeFile.mock.calls[0][1] as string
114+
const writtenData = yaml.parse(writtenContent)
115+
116+
// Should contain only one mode with updated content
117+
expect(writtenData.customModes).toHaveLength(1)
118+
expect(writtenData.customModes[0].slug).toBe("test")
119+
expect(writtenData.customModes[0].name).toBe("Test Mode") // New name
120+
})
121+
})
122+
123+
describe("installMcp", () => {
124+
const mockMcpItem: MarketplaceItem = {
125+
id: "test-mcp",
126+
name: "Test MCP",
127+
description: "A test MCP server for testing",
128+
type: "mcp",
129+
content: JSON.stringify({
130+
command: "test-server",
131+
args: ["--test"],
132+
}),
133+
}
134+
135+
it("should install MCP when mcp.json file does not exist", async () => {
136+
const notFoundError = new Error("File not found") as any
137+
notFoundError.code = "ENOENT"
138+
mockFs.readFile.mockRejectedValueOnce(notFoundError)
139+
mockFs.writeFile.mockResolvedValueOnce(undefined as any)
140+
141+
const result = await installer.installItem(mockMcpItem, { target: "project" })
142+
143+
expect(result.filePath).toBe(path.join("/test/workspace", ".roo", "mcp.json"))
144+
expect(mockFs.writeFile).toHaveBeenCalled()
145+
146+
// Verify the written content contains the new server
147+
const writtenContent = mockFs.writeFile.mock.calls[0][1] as string
148+
const writtenData = JSON.parse(writtenContent)
149+
expect(writtenData.mcpServers["test-mcp"]).toBeDefined()
150+
})
151+
152+
it("should throw error when mcp.json contains invalid JSON", async () => {
153+
const invalidJson = '{ "mcpServers": { invalid json'
154+
155+
mockFs.readFile.mockResolvedValueOnce(invalidJson)
156+
157+
await expect(installer.installItem(mockMcpItem, { target: "project" })).rejects.toThrow(
158+
"Cannot install MCP server: The .roo/mcp.json file contains invalid JSON",
159+
)
160+
161+
// Should NOT write to file
162+
expect(mockFs.writeFile).not.toHaveBeenCalled()
163+
})
164+
165+
it("should install MCP when mcp.json contains valid JSON", async () => {
166+
const existingContent = JSON.stringify({
167+
mcpServers: {
168+
"existing-server": { command: "existing", args: [] },
169+
},
170+
})
171+
172+
mockFs.readFile.mockResolvedValueOnce(existingContent)
173+
mockFs.writeFile.mockResolvedValueOnce(undefined as any)
174+
175+
await installer.installItem(mockMcpItem, { target: "project" })
176+
177+
const writtenContent = mockFs.writeFile.mock.calls[0][1] as string
178+
const writtenData = JSON.parse(writtenContent)
179+
180+
// Should contain both existing and new server
181+
expect(Object.keys(writtenData.mcpServers)).toHaveLength(2)
182+
expect(writtenData.mcpServers["existing-server"]).toBeDefined()
183+
expect(writtenData.mcpServers["test-mcp"]).toBeDefined()
184+
})
185+
})
186+
187+
describe("removeMode", () => {
188+
const mockModeItem: MarketplaceItem = {
189+
id: "test-mode",
190+
name: "Test Mode",
191+
description: "A test mode for testing",
192+
type: "mode",
193+
content: yaml.stringify({
194+
slug: "test",
195+
name: "Test Mode",
196+
roleDefinition: "Test role",
197+
groups: ["read"],
198+
}),
199+
}
200+
201+
it("should throw error when .roomodes contains invalid YAML during removal", async () => {
202+
const invalidYaml = "invalid: yaml: content: {"
203+
204+
mockFs.readFile.mockResolvedValueOnce(invalidYaml)
205+
206+
await expect(installer.removeItem(mockModeItem, { target: "project" })).rejects.toThrow(
207+
"Cannot remove mode: The .roomodes file contains invalid YAML",
208+
)
209+
210+
// Should NOT write to file
211+
expect(mockFs.writeFile).not.toHaveBeenCalled()
212+
})
213+
214+
it("should do nothing when file does not exist", async () => {
215+
const notFoundError = new Error("File not found") as any
216+
notFoundError.code = "ENOENT"
217+
mockFs.readFile.mockRejectedValueOnce(notFoundError)
218+
219+
// Should not throw
220+
await installer.removeItem(mockModeItem, { target: "project" })
221+
222+
expect(mockFs.writeFile).not.toHaveBeenCalled()
223+
})
224+
})
225+
})

‎src/shared/ExtensionMessage.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ export interface ExtensionMessage {
7474
| "indexingStatusUpdate"
7575
| "indexCleared"
7676
| "codebaseIndexConfig"
77+
| "marketplaceInstallResult"
7778
text?: string
7879
payload?: any // Add a generic payload for now, can refine later
7980
action?:

‎src/shared/WebviewMessage.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,7 @@ export interface WebviewMessage {
159159
| "installMarketplaceItemWithParameters"
160160
| "cancelMarketplaceInstall"
161161
| "removeInstalledMarketplaceItem"
162+
| "marketplaceInstallResult"
162163
| "switchTab"
163164
text?: string
164165
tab?: "settings" | "history" | "mcp" | "modes" | "chat" | "marketplace" | "account"

‎webview-ui/src/components/marketplace/components/MarketplaceInstallModal.tsx‎

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import React, { useState, useMemo } from "react"
1+
import React, { useState, useMemo, useEffect } from "react"
22
import { MarketplaceItem, McpParameter, McpInstallationMethod } from "../../../../../src/services/marketplace/types"
33
import { vscode } from "@/utils/vscode"
44
import { useAppTranslation } from "@/i18n/TranslationContext"
@@ -109,6 +109,27 @@ export const MarketplaceInstallModal: React.FC<MarketplaceInstallModalProps> = (
109109
}
110110
}, [item, selectedMethodIndex])
111111

112+
// Listen for installation result messages
113+
useEffect(() => {
114+
const handleMessage = (event: MessageEvent) => {
115+
const message = event.data
116+
if (message.type === "marketplaceInstallResult" && message.slug === item?.id) {
117+
if (message.success) {
118+
// Installation succeeded - show success state
119+
setInstallationComplete(true)
120+
setValidationError(null)
121+
} else {
122+
// Installation failed - show error
123+
setValidationError(message.error || "Installation failed")
124+
setInstallationComplete(false)
125+
}
126+
}
127+
}
128+
129+
window.addEventListener("message", handleMessage)
130+
return () => window.removeEventListener("message", handleMessage)
131+
}, [item?.id])
132+
112133
const handleInstall = () => {
113134
if (!item) return
114135

@@ -145,8 +166,8 @@ export const MarketplaceInstallModal: React.FC<MarketplaceInstallModalProps> = (
145166
},
146167
})
147168

148-
// Show post-installation options
149-
setInstallationComplete(true)
169+
// Don't show success immediately - wait for backend result
170+
// The success state will be shown when installation actually succeeds
150171
setValidationError(null)
151172
}
152173

0 commit comments

Comments
 (0)