Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
101 changes: 100 additions & 1 deletion src/core/config/ConfigManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ export interface ApiConfigData {
[key: string]: ApiConfiguration
}
modeApiConfigs?: Partial<Record<Mode, string>>
migrations?: {
rateLimitMigrated?: boolean // Flag to track if rate limit migration has been applied
}
}

export class ConfigManager {
Expand All @@ -17,8 +20,12 @@ export class ConfigManager {
apiConfigs: {
default: {
id: this.generateId(),
rateLimitSeconds: 0, // Set default rate limit for new installations
},
},
migrations: {
rateLimitMigrated: true, // Mark as migrated for fresh installs
},
}

private readonly SCOPE_PREFIX = "roo_cline_config_"
Expand Down Expand Up @@ -52,15 +59,28 @@ export class ConfigManager {
return
}

// Migrate: ensure all configs have IDs
// Initialize migrations tracking object if it doesn't exist
if (!config.migrations) {
config.migrations = {}
}

let needsMigration = false

// Migrate: ensure all configs have IDs
for (const [name, apiConfig] of Object.entries(config.apiConfigs)) {
if (!apiConfig.id) {
apiConfig.id = this.generateId()
needsMigration = true
}
}

// Apply rate limit migration if needed
if (!config.migrations.rateLimitMigrated) {
await this.migrateRateLimit(config)
config.migrations.rateLimitMigrated = true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should clear out the global state rate limit too right?

needsMigration = true
}

if (needsMigration) {
await this.writeConfig(config)
}
Expand All @@ -70,6 +90,43 @@ export class ConfigManager {
}
}

/**
* Migrate rate limit settings from global state to per-profile configuration
*/
private async migrateRateLimit(config: ApiConfigData): Promise<void> {
try {
// Get the global rate limit value from extension state
let rateLimitSeconds: number | undefined

try {
// Try to get global state rate limit
rateLimitSeconds = await this.context.globalState.get<number>("rateLimitSeconds")
console.log(`[RateLimitMigration] Found global rate limit value: ${rateLimitSeconds}`)
} catch (error) {
console.error("[RateLimitMigration] Error getting global rate limit:", error)
}

// If no global rate limit, use default value of 5 seconds
if (rateLimitSeconds === undefined) {
rateLimitSeconds = 5 // Default value
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default should be 0

console.log(`[RateLimitMigration] Using default rate limit value: ${rateLimitSeconds}`)
}

// Apply the rate limit to all API configurations
for (const [name, apiConfig] of Object.entries(config.apiConfigs)) {
// Only set if not already configured
if (apiConfig.rateLimitSeconds === undefined) {
console.log(`[RateLimitMigration] Applying rate limit ${rateLimitSeconds}s to profile: ${name}`)
apiConfig.rateLimitSeconds = rateLimitSeconds
}
}

console.log(`[RateLimitMigration] Migration complete`)
} catch (error) {
console.error(`[RateLimitMigration] Failed to migrate rate limit settings:`, error)
}
}

/**
* List all available configs with metadata
*/
Expand All @@ -96,6 +153,48 @@ export class ConfigManager {
return await this.lock(async () => {
const currentConfig = await this.readConfig()
const existingConfig = currentConfig.apiConfigs[name]

// If this is a new config or doesn't have rate limit, try to apply the global rate limit
if (!existingConfig || config.rateLimitSeconds === undefined) {
// Apply rate limit if not specified explicitly in the config being saved
if (config.rateLimitSeconds === undefined) {
let globalRateLimit: number | undefined

// First check if we have an existing migrated config to copy from
const anyExistingConfig = Object.values(currentConfig.apiConfigs)[0]
if (anyExistingConfig?.rateLimitSeconds !== undefined) {
globalRateLimit = anyExistingConfig.rateLimitSeconds
console.log(
`[RateLimitMigration] Using existing profile's rate limit value: ${globalRateLimit}s`,
)
} else {
// Otherwise check global state
try {
globalRateLimit = await this.context.globalState.get<number>("rateLimitSeconds")
console.log(
`[RateLimitMigration] Using global rate limit for new profile: ${globalRateLimit}s`,
)
} catch (error) {
console.error(
"[RateLimitMigration] Error getting global rate limit for new profile:",
error,
)
}

// Use default if not found
if (globalRateLimit === undefined) {
globalRateLimit = 5 // Default value
console.log(
`[RateLimitMigration] Using default rate limit value for new profile: ${globalRateLimit}s`,
)
}
}

// Apply the rate limit to the new config
config.rateLimitSeconds = globalRateLimit
}
}
Comment on lines +157 to +196
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't need this code. After we do the migration, we should just remove the idea of a global rate limit.


currentConfig.apiConfigs[name] = {
...config,
id: existingConfig?.id || this.generateId(),
Expand Down
54 changes: 34 additions & 20 deletions src/core/config/__tests__/ConfigManager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,14 @@ const mockSecrets = {
delete: jest.fn(),
}

const mockGlobalState = {
get: jest.fn(),
update: jest.fn(),
}

const mockContext = {
secrets: mockSecrets,
globalState: mockGlobalState,
} as unknown as ExtensionContext

describe("ConfigManager", () => {
Expand Down Expand Up @@ -40,8 +46,12 @@ describe("ConfigManager", () => {
default: {
config: {},
id: "default",
rateLimitSeconds: 5,
},
},
migrations: {
rateLimitMigrated: true,
},
}),
)

Expand All @@ -66,6 +76,9 @@ describe("ConfigManager", () => {
}),
)

// Mock global state to prevent rate limit errors
mockGlobalState.get.mockResolvedValue(5)

await configManager.initConfig()

// Should have written the config with new IDs
Expand Down Expand Up @@ -141,6 +154,9 @@ describe("ConfigManager", () => {

describe("SaveConfig", () => {
it("should save new config", async () => {
// Mock globalState to prevent rate limit errors
mockGlobalState.get.mockResolvedValue(5)

mockSecrets.get.mockResolvedValue(
JSON.stringify({
currentApiConfigName: "default",
Expand All @@ -162,9 +178,10 @@ describe("ConfigManager", () => {

await configManager.saveConfig("test", newConfig)

// Get the actual stored config to check the generated ID
// Get the actual stored config to check the generated ID and rate limit
const storedConfig = JSON.parse(mockSecrets.store.mock.calls[0][1])
const testConfigId = storedConfig.apiConfigs.test.id
const rateLimitSeconds = storedConfig.apiConfigs.test.rateLimitSeconds

const expectedConfig = {
currentApiConfigName: "default",
Expand All @@ -173,6 +190,7 @@ describe("ConfigManager", () => {
test: {
...newConfig,
id: testConfigId,
rateLimitSeconds,
},
},
modeApiConfigs: {
Expand All @@ -196,6 +214,7 @@ describe("ConfigManager", () => {
apiProvider: "anthropic",
apiKey: "old-key",
id: "test-id",
rateLimitSeconds: 5,
},
},
}
Expand All @@ -209,21 +228,14 @@ describe("ConfigManager", () => {

await configManager.saveConfig("test", updatedConfig)

const expectedConfig = {
currentApiConfigName: "default",
apiConfigs: {
test: {
apiProvider: "anthropic",
apiKey: "new-key",
id: "test-id",
},
},
}

expect(mockSecrets.store).toHaveBeenCalledWith(
"roo_cline_config_api_config",
JSON.stringify(expectedConfig, null, 2),
)
// Check that the last call to store has the correct values
const storedConfig = JSON.parse(mockSecrets.store.mock.calls[mockSecrets.store.mock.calls.length - 1][1])
expect(storedConfig.apiConfigs.test).toEqual({
apiProvider: "anthropic",
apiKey: "new-key",
id: "test-id",
rateLimitSeconds: 5,
})
})

it("should throw error if secrets storage fails", async () => {
Expand Down Expand Up @@ -319,8 +331,9 @@ describe("ConfigManager", () => {
id: "test-id",
})

// Get the stored config to check the structure
const storedConfig = JSON.parse(mockSecrets.store.mock.calls[0][1])
// Get the last stored config to check the structure
const lastCallIndex = mockSecrets.store.mock.calls.length - 1
const storedConfig = JSON.parse(mockSecrets.store.mock.calls[lastCallIndex][1])
expect(storedConfig.currentApiConfigName).toBe("test")
expect(storedConfig.apiConfigs.test).toEqual({
apiProvider: "anthropic",
Expand Down Expand Up @@ -386,8 +399,9 @@ describe("ConfigManager", () => {

await configManager.setCurrentConfig("test")

// Get the stored config to check the structure
const storedConfig = JSON.parse(mockSecrets.store.mock.calls[0][1])
// Get the last stored config to check the structure
const lastCallIndex = mockSecrets.store.mock.calls.length - 1
const storedConfig = JSON.parse(mockSecrets.store.mock.calls[lastCallIndex][1])
expect(storedConfig.currentApiConfigName).toBe("test")
expect(storedConfig.apiConfigs.default.id).toBe("default")
expect(storedConfig.apiConfigs.test).toEqual({
Expand Down
Loading