Skip to content

fix: Replace axios with fetch in RemoteConfigLoader for HTTP proxy support #10534

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
roomote wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix: Replace axios with fetch in RemoteConfigLoader for HTTP proxy support #10534

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions src/services/marketplace/RemoteConfigLoader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import axios from "axios"
import * as yaml from "yaml"
import { z } from "zod"

Expand Down Expand Up @@ -88,14 +87,28 @@ export class RemoteConfigLoader {

for (let i = 0; i < maxRetries; i++) {
try {
const response = await axios.get(url, {
timeout: 10000, // 10 second timeout
// Use AbortController for timeout - fetch does not have built-in timeout
const controller = new AbortController()
const timeoutId = setTimeout(() => controller.abort(), 10000) // 10 second timeout

const response = await fetch(url, {
signal: controller.signal,
headers: {
Accept: "application/json",
"Content-Type": "application/json",
},
})
return response.data as T

clearTimeout(timeoutId)

if (!response.ok) {
throw new Error(`HTTP error! status: ${response.status}`)
}

// Using fetch with native fetch API in VS Code automatically respects
// VS Code's http.proxy settings, unlike axios which requires manual
// proxy agent configuration
return (await response.text()) as T
} catch (error) {
lastError = error as Error
if (i < maxRetries - 1) {
Expand Down
3 changes: 0 additions & 3 deletions src/services/marketplace/__tests__/MarketplaceManager.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,6 @@ vi.mock("@roo-code/cloud", () => ({
},
}))

// Mock axios
vi.mock("axios")

// Mock TelemetryService
vi.mock("../../../../packages/telemetry/src/TelemetryService", () => ({
TelemetryService: {
Expand Down
140 changes: 95 additions & 45 deletions src/services/marketplace/__tests__/RemoteConfigLoader.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,30 @@
// npx vitest services/marketplace/__tests__/RemoteConfigLoader.spec.ts

import axios from "axios"
import { RemoteConfigLoader } from "../RemoteConfigLoader"
import type { MarketplaceItemType } from "@roo-code/types"

// Mock axios
vi.mock("axios")
const mockedAxios = axios as any

// Mock the cloud config
vi.mock("@roo-code/cloud", () => ({
getRooCodeApiUrl: () => "https://test.api.com",
}))

describe("RemoteConfigLoader", () => {
let loader: RemoteConfigLoader
let mockFetch: ReturnType<typeof vi.fn>

beforeEach(() => {
loader = new RemoteConfigLoader()
vi.clearAllMocks()
// Clear any existing cache
loader.clearCache()

// Reset fetch mock
mockFetch = vi.fn()
global.fetch = mockFetch
})

afterEach(() => {
vi.restoreAllMocks()
})

describe("loadAllItems", () => {
Expand All @@ -38,33 +42,37 @@ describe("RemoteConfigLoader", () => {
url: "https://github.com/test/test-mcp"
content: '{"command": "test"}'`

mockedAxios.get.mockImplementation((url: string) => {
mockFetch.mockImplementation((url: string) => {
if (url.includes("/modes")) {
return Promise.resolve({ data: mockModesYaml })
return Promise.resolve({
ok: true,
text: () => Promise.resolve(mockModesYaml),
})
}
if (url.includes("/mcps")) {
return Promise.resolve({ data: mockMcpsYaml })
return Promise.resolve({
ok: true,
text: () => Promise.resolve(mockMcpsYaml),
})
}
return Promise.reject(new Error("Unknown URL"))
})

const items = await loader.loadAllItems()

expect(mockedAxios.get).toHaveBeenCalledTimes(2)
expect(mockedAxios.get).toHaveBeenCalledWith(
expect(mockFetch).toHaveBeenCalledTimes(2)
expect(mockFetch).toHaveBeenCalledWith(
"https://test.api.com/api/marketplace/modes",
expect.objectContaining({
timeout: 10000,
headers: {
Accept: "application/json",
"Content-Type": "application/json",
},
}),
)
expect(mockedAxios.get).toHaveBeenCalledWith(
expect(mockFetch).toHaveBeenCalledWith(
"https://test.api.com/api/marketplace/mcps",
expect.objectContaining({
timeout: 10000,
headers: {
Accept: "application/json",
"Content-Type": "application/json",
Expand Down Expand Up @@ -104,23 +112,29 @@ describe("RemoteConfigLoader", () => {
url: "https://github.com/test/test-mcp"
content: "test content"`

mockedAxios.get.mockImplementation((url: string) => {
mockFetch.mockImplementation((url: string) => {
if (url.includes("/modes")) {
return Promise.resolve({ data: mockModesYaml })
return Promise.resolve({
ok: true,
text: () => Promise.resolve(mockModesYaml),
})
}
if (url.includes("/mcps")) {
return Promise.resolve({ data: mockMcpsYaml })
return Promise.resolve({
ok: true,
text: () => Promise.resolve(mockMcpsYaml),
})
}
return Promise.reject(new Error("Unknown URL"))
})

// First call - should hit API
const items1 = await loader.loadAllItems()
expect(mockedAxios.get).toHaveBeenCalledTimes(2)
expect(mockFetch).toHaveBeenCalledTimes(2)

// Second call - should use cache
const items2 = await loader.loadAllItems()
expect(mockedAxios.get).toHaveBeenCalledTimes(2) // Still 2, not 4
expect(mockFetch).toHaveBeenCalledTimes(2) // Still 2, not 4

expect(items1).toEqual(items2)
})
Expand All @@ -136,16 +150,22 @@ describe("RemoteConfigLoader", () => {

// Mock modes endpoint to fail twice then succeed
let modesCallCount = 0
mockedAxios.get.mockImplementation((url: string) => {
mockFetch.mockImplementation((url: string) => {
if (url.includes("/modes")) {
modesCallCount++
if (modesCallCount <= 2) {
return Promise.reject(new Error("Network error"))
}
return Promise.resolve({ data: mockModesYaml })
return Promise.resolve({
ok: true,
text: () => Promise.resolve(mockModesYaml),
})
}
if (url.includes("/mcps")) {
return Promise.resolve({ data: mockMcpsYaml })
return Promise.resolve({
ok: true,
text: () => Promise.resolve(mockMcpsYaml),
})
}
return Promise.reject(new Error("Unknown URL"))
})
Expand All @@ -159,18 +179,15 @@ describe("RemoteConfigLoader", () => {
})

it("should throw error after max retries", async () => {
mockedAxios.get.mockRejectedValue(new Error("Persistent network error"))
mockFetch.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),
)
expect(mockFetch).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)
expect(mockFetch.mock.calls.length).toBeGreaterThanOrEqual(2)
})

it("should handle invalid data gracefully", async () => {
Expand All @@ -185,19 +202,34 @@ describe("RemoteConfigLoader", () => {
url: "https://github.com/test/test-mcp"
content: "test content"`

mockedAxios.get.mockImplementation((url: string) => {
mockFetch.mockImplementation((url: string) => {
if (url.includes("/modes")) {
return Promise.resolve({ data: invalidModesYaml })
return Promise.resolve({
ok: true,
text: () => Promise.resolve(invalidModesYaml),
})
}
if (url.includes("/mcps")) {
return Promise.resolve({ data: validMcpsYaml })
return Promise.resolve({
ok: true,
text: () => Promise.resolve(validMcpsYaml),
})
}
return Promise.reject(new Error("Unknown URL"))
})

// Should throw validation error for invalid modes
await expect(loader.loadAllItems()).rejects.toThrow()
})

it("should handle HTTP error responses", async () => {
mockFetch.mockResolvedValue({
ok: false,
status: 500,
})

await expect(loader.loadAllItems()).rejects.toThrow("HTTP error! status: 500")
})
})

describe("getItem", () => {
Expand All @@ -215,12 +247,18 @@ describe("RemoteConfigLoader", () => {
url: "https://github.com/test/test-mcp"
content: "test content"`

mockedAxios.get.mockImplementation((url: string) => {
mockFetch.mockImplementation((url: string) => {
if (url.includes("/modes")) {
return Promise.resolve({ data: mockModesYaml })
return Promise.resolve({
ok: true,
text: () => Promise.resolve(mockModesYaml),
})
}
if (url.includes("/mcps")) {
return Promise.resolve({ data: mockMcpsYaml })
return Promise.resolve({
ok: true,
text: () => Promise.resolve(mockMcpsYaml),
})
}
return Promise.reject(new Error("Unknown URL"))
})
Expand Down Expand Up @@ -260,30 +298,36 @@ describe("RemoteConfigLoader", () => {

const mockMcpsYaml = `items: []`

mockedAxios.get.mockImplementation((url: string) => {
mockFetch.mockImplementation((url: string) => {
if (url.includes("/modes")) {
return Promise.resolve({ data: mockModesYaml })
return Promise.resolve({
ok: true,
text: () => Promise.resolve(mockModesYaml),
})
}
if (url.includes("/mcps")) {
return Promise.resolve({ data: mockMcpsYaml })
return Promise.resolve({
ok: true,
text: () => Promise.resolve(mockMcpsYaml),
})
}
return Promise.reject(new Error("Unknown URL"))
})

// First call
await loader.loadAllItems()
expect(mockedAxios.get).toHaveBeenCalledTimes(2)
expect(mockFetch).toHaveBeenCalledTimes(2)

// Second call - should use cache
await loader.loadAllItems()
expect(mockedAxios.get).toHaveBeenCalledTimes(2)
expect(mockFetch).toHaveBeenCalledTimes(2)

// Clear cache
loader.clearCache()

// Third call - should hit API again
await loader.loadAllItems()
expect(mockedAxios.get).toHaveBeenCalledTimes(4)
expect(mockFetch).toHaveBeenCalledTimes(4)
})
})

Expand All @@ -297,12 +341,18 @@ describe("RemoteConfigLoader", () => {

const mockMcpsYaml = `items: []`

mockedAxios.get.mockImplementation((url: string) => {
mockFetch.mockImplementation((url: string) => {
if (url.includes("/modes")) {
return Promise.resolve({ data: mockModesYaml })
return Promise.resolve({
ok: true,
text: () => Promise.resolve(mockModesYaml),
})
}
if (url.includes("/mcps")) {
return Promise.resolve({ data: mockMcpsYaml })
return Promise.resolve({
ok: true,
text: () => Promise.resolve(mockMcpsYaml),
})
}
return Promise.reject(new Error("Unknown URL"))
})
Expand All @@ -315,18 +365,18 @@ describe("RemoteConfigLoader", () => {

// First call
await loader.loadAllItems()
expect(mockedAxios.get).toHaveBeenCalledTimes(2)
expect(mockFetch).toHaveBeenCalledTimes(2)

// Second call immediately - should use cache
await loader.loadAllItems()
expect(mockedAxios.get).toHaveBeenCalledTimes(2)
expect(mockFetch).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)
expect(mockFetch).toHaveBeenCalledTimes(4)

// Restore original Date.now
Date.now = originalDateNow
Expand Down
Loading