-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: Allow scheduling model switches during execution #8335
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.
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
Conversation
- Modified ApiConfigSelector to remain interactive when disabled - Added scheduledConfigId and onScheduleChange props - Implemented visual indicators (clock icons) for scheduled switches - Added state management in ChatTextArea for tracking scheduled config - Automatically applies scheduled switch when request completes - Added comprehensive unit tests for the new functionality - Added translation keys for new UI elements Fixes #8334
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-review: debating with my own diff like a deterministic rubber duck.
| @@ -0,0 +1 @@ | |||
| Subproject commit e46929b8d8add0cd3c412d69f8ac882c405a4ba9 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P0: This appears to add a git submodule (mode 160000). This is likely unintended and will introduce a submodule pointer into the repo. Please remove this entry from the PR (drop the submodule file or reset the tree to exclude it).
| @@ -0,0 +1 @@ | |||
| Subproject commit 88a473b017af37091c85ce3056e444e856f80d6e | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P0: Another unintended git submodule pointer. This will create a submodule under tmp/. Please remove this from the PR to avoid repo pollution and CI/tooling issues.
| } | ||
| } else { | ||
| // Apply immediately when not disabled | ||
| onChange(configId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: When disabled is true but onScheduleChange is undefined, selections will still call onChange(configId), applying the switch immediately during execution. That breaks the scheduling-only behavior. Consider guarding this path.
| onChange(configId) | |
| } else { | |
| // Apply immediately when not disabled; but if disabled without scheduler, ignore | |
| if (disabled) { | |
| return | |
| } | |
| onChange(configId) | |
| } |
| content={scheduledConfigName && disabled ? `${title} (Scheduled: ${scheduledConfigName})` : title}> | ||
| <PopoverTrigger | ||
| disabled={disabled} | ||
| disabled={false} // Always allow opening the dropdown |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Accessibility. The trigger is intentionally kept enabled (disabled={false}) to allow scheduling while execution is in progress, but it lacks aria-disabled. Adding aria-disabled preserves semantics for assistive tech while retaining interactivity.
| disabled={false} // Always allow opening the dropdown | |
| disabled={false} aria-disabled={disabled} // Always allow opening the dropdown |
| const handleSelect = useCallback( | ||
| (configId: string) => { | ||
| onChange(configId) | ||
| if (disabled && onScheduleChange) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: UX edge case. If a user selects the current model while disabled (no schedule yet), we end up “scheduling” the same model. Prefer treating this as a no-op (or cancel any existing schedule). This improves clarity and avoids a clock icon that implies a change that won’t happen.
| if (disabled && onScheduleChange) { | |
| if (disabled && onScheduleChange) { | |
| // When disabled, schedule the change instead of applying immediately | |
| if (configId === value) { | |
| // Prevent scheduling the current model | |
| onScheduleChange(undefined) | |
| setOpen(false) | |
| setSearchValue("") | |
| return | |
| } | |
| if (scheduledConfigId === configId) { | |
| // If clicking the same config, cancel the scheduled change | |
| onScheduleChange(undefined) | |
| } else { | |
| // Schedule the new config | |
| onScheduleChange(configId) | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (disabled && onScheduleChange) { | |
| if (disabled && onScheduleChange) { |
| ...defaultProps, | ||
| pinnedApiConfigs: { config1: true, config3: true }, | ||
| } | ||
| describe("Scheduled Model Switch", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P3: Recommend adding a test for the case where disabled === true and onScheduleChange is not provided to ensure no immediate onChange occurs (selection should be ignored or require scheduler).
This PR addresses Issue #8334 by enabling users to schedule model switches while Roo is actively processing a request.
Summary
Currently, the model selector dropdown is completely disabled during execution, preventing users from changing models until the current request completes. This PR implements a scheduling mechanism that allows users to queue a model switch that will automatically apply after the current request finishes.
Changes
Core Implementation
scheduledConfigIdandonScheduleChangepropsUser Experience
Testing & Quality
Testing Instructions
Screenshots
The implementation adds subtle visual indicators without disrupting the existing UI:
Fixes #8334
Important
Introduces model switch scheduling during execution with UI indicators and state management in
ApiConfigSelector.tsx, tested inApiConfigSelector.spec.tsx.ApiConfigSelector.tsx.scheduledConfigIdandonScheduleChangeprops for state management.ApiConfigSelector.spec.tsxfor new scheduling functionality.chat.jsonandprompts.jsonfor internationalization.This description was created by
for e68076e. You can customize this summary. It will automatically update as commits are pushed.