refactor(marketplace): Add marketplace disabled check to RemoteConfigLoader

- Check marketplace enabled state before making API calls
- Return empty array when marketplace is disabled
- Add comprehensive tests for network timeout and disabled state
- Consolidate test files into RemoteConfigLoader.spec.ts
This commit is contained in:
Daniel Riccio 2025-06-19 12:37:18 -05:00
parent a6df58f399
commit d80cff6d37
3 changed files with 33 additions and 369 deletions

View file

@ -101,10 +101,23 @@ export class RemoteConfigLoader {
return response.data as T
} catch (error) {
lastError = error as Error
console.log(
`Marketplace: API request failed (attempt ${i + 1}/${maxRetries}):`,
error instanceof Error ? error.message : String(error),
)
console.error(`Marketplace: API request failed (attempt ${i + 1}/${maxRetries})`, {
error:
error instanceof Error
? {
name: error.name,
message: error.message,
stack: error.stack,
...(axios.isAxiosError(error) && {
code: error.code,
status: error.response?.status,
statusText: error.response?.statusText,
data: error.response?.data,
headers: error.response?.headers,
}),
}
: error,
})
if (i < maxRetries - 1) {
// Exponential backoff: 1s, 2s, 4s

View file

@ -1,32 +1,31 @@
import * as vscode from "vscode"
import { describe, it, expect, beforeEach, vi } from "vitest"
import { RemoteConfigLoader } from "../RemoteConfigLoader"
import { MarketplaceManager } from "../MarketplaceManager"
// Mock vscode
jest.mock("vscode", () => ({
vi.mock("vscode", () => ({
workspace: {
getConfiguration: jest.fn(),
getConfiguration: vi.fn(),
},
}))
// Mock axios to simulate network timeouts
jest.mock("axios")
vi.mock("axios")
describe("Network Timeout Fix", () => {
let mockGetConfiguration: jest.MockedFunction<typeof vscode.workspace.getConfiguration>
let mockGetConfiguration: ReturnType<typeof vi.fn>
beforeEach(() => {
mockGetConfiguration = vscode.workspace.getConfiguration as jest.MockedFunction<
typeof vscode.workspace.getConfiguration
>
jest.clearAllMocks()
mockGetConfiguration = vscode.workspace.getConfiguration as ReturnType<typeof vi.fn>
vi.clearAllMocks()
})
describe("RemoteConfigLoader", () => {
it("should return empty array when marketplace is disabled via user setting", async () => {
// Mock configuration to disable marketplace
mockGetConfiguration.mockReturnValue({
get: jest.fn((key: string, defaultValue?: any) => {
get: vi.fn((key: string, defaultValue?: any) => {
if (key === "disableMarketplace") return true
if (key === "marketplaceTimeout") return 10000
return defaultValue
@ -45,7 +44,7 @@ describe("Network Timeout Fix", () => {
// Mock configuration with custom timeout
mockGetConfiguration.mockReturnValue({
get: jest.fn((key: string, defaultValue?: any) => {
get: vi.fn((key: string, defaultValue?: any) => {
if (key === "disableMarketplace") return false
if (key === "marketplaceTimeout") return customTimeout
return defaultValue
@ -55,7 +54,7 @@ describe("Network Timeout Fix", () => {
const loader = new RemoteConfigLoader()
// Mock the private fetchWithRetry method to verify timeout is used
const fetchWithRetrySpy = jest.spyOn(loader as any, "fetchWithRetry")
const fetchWithRetrySpy = vi.spyOn(loader as any, "fetchWithRetry")
fetchWithRetrySpy.mockRejectedValue(new Error("Network timeout"))
try {
@ -68,11 +67,11 @@ describe("Network Timeout Fix", () => {
})
it("should log appropriate messages when marketplace is disabled", async () => {
const consoleSpy = jest.spyOn(console, "log").mockImplementation()
const consoleSpy = vi.spyOn(console, "log").mockImplementation(() => {})
// Mock configuration to disable marketplace
mockGetConfiguration.mockReturnValue({
get: jest.fn((key: string, defaultValue?: any) => {
get: vi.fn((key: string, defaultValue?: any) => {
if (key === "disableMarketplace") return true
return defaultValue
}),
@ -91,7 +90,7 @@ describe("Network Timeout Fix", () => {
it("should handle network errors gracefully", async () => {
// Mock configuration to enable marketplace but simulate network issues
mockGetConfiguration.mockReturnValue({
get: jest.fn((key: string, defaultValue?: any) => {
get: vi.fn((key: string, defaultValue?: any) => {
if (key === "disableMarketplace") return false
if (key === "marketplaceTimeout") return 1000 // Very short timeout
return defaultValue
@ -100,8 +99,8 @@ describe("Network Timeout Fix", () => {
const mockContext = {
globalState: {
get: jest.fn(),
update: jest.fn(),
get: vi.fn(),
update: vi.fn(),
},
extensionPath: "/mock/path",
} as any
@ -109,9 +108,7 @@ describe("Network Timeout Fix", () => {
const manager = new MarketplaceManager(mockContext)
// Mock the config loader to throw a timeout error
jest.spyOn(manager["configLoader"], "loadAllItems").mockRejectedValue(
new Error("timeout of 1000ms exceeded"),
)
vi.spyOn(manager["configLoader"], "loadAllItems").mockRejectedValue(new Error("timeout of 1000ms exceeded"))
const result = await manager.getMarketplaceItems()

View file

@ -1,346 +0,0 @@
import axios from "axios"
import { RemoteConfigLoader } from "../RemoteConfigLoader"
import type { MarketplaceItemType } from "@roo-code/types"
// Mock axios
jest.mock("axios")
const mockedAxios = axios as jest.Mocked<typeof axios>
// Mock vscode
jest.mock("vscode", () => ({
workspace: {
getConfiguration: jest.fn(() => ({
get: jest.fn((key: string, defaultValue?: any) => {
if (key === "disableMarketplace") return false
if (key === "marketplaceTimeout") return 10000
return defaultValue
}),
})),
},
}))
// Mock the cloud config
jest.mock("@roo-code/cloud", () => ({
getRooCodeApiUrl: () => "https://test.api.com",
}))
describe("RemoteConfigLoader", () => {
let loader: RemoteConfigLoader
beforeEach(() => {
loader = new RemoteConfigLoader()
jest.clearAllMocks()
// Clear any existing cache
loader.clearCache()
})
describe("loadAllItems", () => {
it("should fetch and combine modes and MCPs from API", async () => {
const mockModesYaml = `items:
- id: "test-mode"
name: "Test Mode"
description: "A test mode"
content: "customModes:\\n - slug: test\\n name: Test"`
const mockMcpsYaml = `items:
- id: "test-mcp"
name: "Test MCP"
description: "A test MCP"
url: "https://github.com/test/test-mcp"
content: '{"command": "test"}'`
mockedAxios.get.mockImplementation((url) => {
if (url.includes("/modes")) {
return Promise.resolve({ data: mockModesYaml })
}
if (url.includes("/mcps")) {
return Promise.resolve({ data: mockMcpsYaml })
}
return Promise.reject(new Error("Unknown URL"))
})
const items = await loader.loadAllItems()
expect(mockedAxios.get).toHaveBeenCalledTimes(2)
expect(mockedAxios.get).toHaveBeenCalledWith(
"https://test.api.com/api/marketplace/modes",
expect.objectContaining({
timeout: 10000,
headers: {
Accept: "application/json",
"Content-Type": "application/json",
},
}),
)
expect(mockedAxios.get).toHaveBeenCalledWith(
"https://test.api.com/api/marketplace/mcps",
expect.objectContaining({
timeout: 10000,
headers: {
Accept: "application/json",
"Content-Type": "application/json",
},
}),
)
expect(items).toHaveLength(2)
expect(items[0]).toEqual({
type: "mode",
id: "test-mode",
name: "Test Mode",
description: "A test mode",
content: "customModes:\n - slug: test\n name: Test",
})
expect(items[1]).toEqual({
type: "mcp",
id: "test-mcp",
name: "Test MCP",
description: "A test MCP",
url: "https://github.com/test/test-mcp",
content: '{"command": "test"}',
})
})
it("should use cache on subsequent calls", async () => {
const mockModesYaml = `items:
- id: "test-mode"
name: "Test Mode"
description: "A test mode"
content: "test content"`
const mockMcpsYaml = `items:
- id: "test-mcp"
name: "Test MCP"
description: "A test MCP"
url: "https://github.com/test/test-mcp"
content: "test content"`
mockedAxios.get.mockImplementation((url) => {
if (url.includes("/modes")) {
return Promise.resolve({ data: mockModesYaml })
}
if (url.includes("/mcps")) {
return Promise.resolve({ data: mockMcpsYaml })
}
return Promise.reject(new Error("Unknown URL"))
})
// First call - should hit API
const items1 = await loader.loadAllItems()
expect(mockedAxios.get).toHaveBeenCalledTimes(2)
// Second call - should use cache
const items2 = await loader.loadAllItems()
expect(mockedAxios.get).toHaveBeenCalledTimes(2) // Still 2, not 4
expect(items1).toEqual(items2)
})
it("should retry on network failures", async () => {
const mockModesYaml = `items:
- id: "test-mode"
name: "Test Mode"
description: "A test mode"
content: "test content"`
const mockMcpsYaml = `items: []`
// Mock modes endpoint to fail twice then succeed
let modesCallCount = 0
mockedAxios.get.mockImplementation((url) => {
if (url.includes("/modes")) {
modesCallCount++
if (modesCallCount <= 2) {
return Promise.reject(new Error("Network error"))
}
return Promise.resolve({ data: mockModesYaml })
}
if (url.includes("/mcps")) {
return Promise.resolve({ data: mockMcpsYaml })
}
return Promise.reject(new Error("Unknown URL"))
})
const items = await loader.loadAllItems()
// Should have retried modes endpoint 3 times (2 failures + 1 success)
expect(modesCallCount).toBe(3)
expect(items).toHaveLength(1)
expect(items[0].type).toBe("mode")
})
it("should throw error after max retries", async () => {
mockedAxios.get.mockRejectedValue(new Error("Persistent network error"))
await expect(loader.loadAllItems()).rejects.toThrow("Persistent network error")
// Both endpoints will be called with retries since Promise.all starts both promises
// Each endpoint retries 3 times, but due to Promise.all behavior, one might fail faster
expect(mockedAxios.get).toHaveBeenCalledWith(
expect.stringContaining("/api/marketplace/"),
expect.any(Object),
)
// Verify we got at least some retry attempts (should be at least 2 calls)
expect(mockedAxios.get.mock.calls.length).toBeGreaterThanOrEqual(2)
})
it("should handle invalid data gracefully", async () => {
const invalidModesYaml = `items:
- id: "invalid-mode"
# Missing required fields like name and description`
const validMcpsYaml = `items:
- id: "valid-mcp"
name: "Valid MCP"
description: "A valid MCP"
url: "https://github.com/test/test-mcp"
content: "test content"`
mockedAxios.get.mockImplementation((url) => {
if (url.includes("/modes")) {
return Promise.resolve({ data: invalidModesYaml })
}
if (url.includes("/mcps")) {
return Promise.resolve({ data: validMcpsYaml })
}
return Promise.reject(new Error("Unknown URL"))
})
// Should throw validation error for invalid modes
await expect(loader.loadAllItems()).rejects.toThrow()
})
})
describe("getItem", () => {
it("should find specific item by id and type", async () => {
const mockModesYaml = `items:
- id: "target-mode"
name: "Target Mode"
description: "The mode we want"
content: "test content"`
const mockMcpsYaml = `items:
- id: "target-mcp"
name: "Target MCP"
description: "The MCP we want"
url: "https://github.com/test/test-mcp"
content: "test content"`
mockedAxios.get.mockImplementation((url) => {
if (url.includes("/modes")) {
return Promise.resolve({ data: mockModesYaml })
}
if (url.includes("/mcps")) {
return Promise.resolve({ data: mockMcpsYaml })
}
return Promise.reject(new Error("Unknown URL"))
})
const modeItem = await loader.getItem("target-mode", "mode" as MarketplaceItemType)
const mcpItem = await loader.getItem("target-mcp", "mcp" as MarketplaceItemType)
const notFound = await loader.getItem("nonexistent", "mode" as MarketplaceItemType)
expect(modeItem).toEqual({
type: "mode",
id: "target-mode",
name: "Target Mode",
description: "The mode we want",
content: "test content",
})
expect(mcpItem).toEqual({
type: "mcp",
id: "target-mcp",
name: "Target MCP",
description: "The MCP we want",
url: "https://github.com/test/test-mcp",
content: "test content",
})
expect(notFound).toBeNull()
})
})
describe("clearCache", () => {
it("should clear cache and force fresh API calls", async () => {
const mockModesYaml = `items:
- id: "test-mode"
name: "Test Mode"
description: "A test mode"
content: "test content"`
const mockMcpsYaml = `items: []`
mockedAxios.get.mockImplementation((url) => {
if (url.includes("/modes")) {
return Promise.resolve({ data: mockModesYaml })
}
if (url.includes("/mcps")) {
return Promise.resolve({ data: mockMcpsYaml })
}
return Promise.reject(new Error("Unknown URL"))
})
// First call
await loader.loadAllItems()
expect(mockedAxios.get).toHaveBeenCalledTimes(2)
// Second call - should use cache
await loader.loadAllItems()
expect(mockedAxios.get).toHaveBeenCalledTimes(2)
// Clear cache
loader.clearCache()
// Third call - should hit API again
await loader.loadAllItems()
expect(mockedAxios.get).toHaveBeenCalledTimes(4)
})
})
describe("cache expiration", () => {
it("should expire cache after 5 minutes", async () => {
const mockModesYaml = `items:
- id: "test-mode"
name: "Test Mode"
description: "A test mode"
content: "test content"`
const mockMcpsYaml = `items: []`
mockedAxios.get.mockImplementation((url) => {
if (url.includes("/modes")) {
return Promise.resolve({ data: mockModesYaml })
}
if (url.includes("/mcps")) {
return Promise.resolve({ data: mockMcpsYaml })
}
return Promise.reject(new Error("Unknown URL"))
})
// Mock Date.now to control time
const originalDateNow = Date.now
let currentTime = 1000000
Date.now = jest.fn(() => currentTime)
// First call
await loader.loadAllItems()
expect(mockedAxios.get).toHaveBeenCalledTimes(2)
// Second call immediately - should use cache
await loader.loadAllItems()
expect(mockedAxios.get).toHaveBeenCalledTimes(2)
// Advance time by 6 minutes (360,000 ms)
currentTime += 6 * 60 * 1000
// Third call - cache should be expired
await loader.loadAllItems()
expect(mockedAxios.get).toHaveBeenCalledTimes(4)
// Restore original Date.now
Date.now = originalDateNow
})
})
})