feat(settings): add rate limit control for Profile configuration #1785
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Introduce a new RateLimitControl component to manage per-profile rate limiting. The API configuration now includes a rateLimitSeconds option, allowing users to set a custom rate limit. This change also updates the AdvancedSettings and ApiOptions components to integrate the new rate limit functionality. Add translations for the new rate limit settings in multiple languages.
|
dosubot
bot
added
size:L
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
olweraltuve
changed the title
Collaborator
|
I think we should do the migration for the current value into all profiles. And are there more places to remove references to the global state? |
Add support for migrating global rate limit settings to individual API profiles. Introduce a migrations object to track migration status and apply rate limits during configuration saves. New tests ensure correct migration behavior for existing and new profiles.
Added mock global state to prevent rate limit errors during tests. Updated assertions to verify rate limit values in stored configurations. This ensures that the ConfigManager behaves correctly when handling rate limits in various scenarios.
Contributor
Author
|
I think everything looks good now; the migration of the previous value is already in place @mrubens |
mrubens
reviewed
Mar 24, 2025
|
|
||
| // If no global rate limit, use default value of 5 seconds | ||
| if (rateLimitSeconds === undefined) { | ||
| rateLimitSeconds = 5 // Default value |
mrubens
reviewed
Mar 24, 2025
| // Apply rate limit migration if needed | ||
| if (!config.migrations.rateLimitMigrated) { | ||
| await this.migrateRateLimit(config) | ||
| config.migrations.rateLimitMigrated = true |
Collaborator
There was a problem hiding this comment.
We should clear out the global state rate limit too right?
mrubens
reviewed
Mar 24, 2025
Comment on lines
+157
to
+196
| // 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 | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
We shouldn't need this code. After we do the migration, we should just remove the idea of a global rate limit.
mrubens
reviewed
Mar 24, 2025
|
|
||
| <Section> | ||
| <div> | ||
| <div className="flex flex-col gap-2"> |
Collaborator
There was a problem hiding this comment.
Is this just an empty section now? Can we get rid of it?
mrubens
reviewed
Mar 24, 2025
| useDebounce(() => onChange(inputValue), 50, [onChange, inputValue]) | ||
| // Sync internal state with prop changes when switching profiles | ||
| useEffect(() => { | ||
| const hasCustomRateLimit = value !== undefined && value !== null |
Collaborator
There was a problem hiding this comment.
This checkbox doesn't seem to be serializing correctly
Screen.Recording.2025-03-24.at.3.10.38.PM.mov
This was referenced Apr 5, 2025
Collaborator
|
Completed in #2376. Thanks for pushing for this! |
github-project-automation
bot
moved this from PR [Pre Approval Review]
to Done
in Roo Code Roadmap
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduce a new RateLimitControl component to manage per-profile rate limiting. The API configuration now includes a rateLimitSeconds option, allowing users to set a custom rate limit. This change also updates the AdvancedSettings and ApiOptions components to integrate the new rate limit functionality.
Add translations for the new rate limit settings in multiple languages.
Important
Adds rate limit control to API configurations with a new
RateLimitControlcomponent and updates translations across multiple languages.rateLimitSecondsoption toApiHandlerOptionsinapi.tsfor per-profile rate limiting.API_CONFIG_KEYSinapi.tsto includerateLimitSeconds.AdvancedSettings.tsx.RateLimitControlinApiOptions.tsx.RateLimitControlcomponent inRateLimitControl.tsxfor managing rate limits.ca/settings.json,de/settings.json,en/settings.json,es/settings.json,fr/settings.json,hi/settings.json,it/settings.json,ja/settings.json,ko/settings.json,pl/settings.json,pt-BR/settings.json,tr/settings.json,vi/settings.json,zh-CN/settings.json,zh-TW/settings.json.This description was created by
for 9efc90d. It will automatically update as commits are pushed.