Skip to content

Conversation

@daniel-lxs
Copy link
Member

@daniel-lxs daniel-lxs commented Aug 14, 2025

Problem

The save button wasn't enabling when users changed provider settings like dropdowns and checkboxes. This was because setApiConfigurationField was treating all changes from undefined to a defined value as 'initial sync' and not marking the form as dirty.

Solution

Added an optional isUserAction parameter (defaults to true) to setApiConfigurationField to distinguish between:

  • User actions (should enable save button) - the default behavior
  • Automatic initialization (shouldn't enable save button) - explicitly pass false

Changes Made

  • Modified setApiConfigurationField in SettingsView.tsx to accept optional isUserAction parameter
  • Updated components that perform automatic initialization to pass isUserAction: false:
    • ModelPicker.tsx - when auto-setting initial model
    • ThinkingBudget.tsx - when auto-adjusting reasoning effort and thinking tokens
    • ApiOptions.tsx - when setting default models during provider switching
    • HuggingFace.tsx - when auto-setting default provider
    • Unbound.tsx - when auto-selecting first available model

What This Fixes

✅ Checkboxes with default values (like openRouterUseMiddleOutTransform ?? true) now enable the save button when clicked
✅ Provider dropdown changes now enable the save button
✅ Any user interaction with settings properly marks the form as dirty
✅ Automatic initialization (like setting default models) won't falsely enable the save button

Testing

  • All TypeScript compilation checks pass
  • All linting checks pass
  • The fix is backward compatible - components that don't need to distinguish between user/auto actions work unchanged

Fixes #[issue-number]


Important

Adds isUserAction parameter to setApiConfigurationField to enable save button for user changes in provider settings.

  • Behavior:
    • Adds isUserAction parameter to setApiConfigurationField in SettingsView.tsx to differentiate user actions from automatic initializations.
    • User actions enable the save button; automatic initializations do not.
  • Components:
    • Updates ModelPicker.tsx, ThinkingBudget.tsx, ApiOptions.tsx, HuggingFace.tsx, and Unbound.tsx to pass isUserAction: false for automatic initializations.
  • Fixes:
    • Enables save button for checkbox and dropdown changes in provider settings.
    • Ensures user interactions mark the form as dirty, while automatic initializations do not.
  • Testing:
    • All TypeScript compilation and linting checks pass.
    • Backward compatibility maintained for components not distinguishing user/auto actions.

This description was created by Ellipsis for 55f7792. You can customize this summary. It will automatically update as commits are pushed.

The save button wasn't enabling when users changed provider settings like
dropdowns and checkboxes because setApiConfigurationField was treating all
changes from undefined to a defined value as 'initial sync' and not marking
the form as dirty.

Added an optional isUserAction parameter (defaults to true) to distinguish:
- User actions (should enable save button) - the default
- Automatic initialization (shouldn't enable save button) - pass false

This fixes the issue where changing provider dropdowns, checkboxes with
default values, and other settings wouldn't enable the save button.
@daniel-lxs daniel-lxs requested review from cte, jr and mrubens as code owners August 14, 2025 23:07
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. bug Something isn't working UI/UX UI/UX related or focused labels Aug 14, 2025
Copy link
Contributor

@roomote roomote bot left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! This PR successfully fixes the save button enablement issue for provider settings. The solution is clean, backward-compatible, and well-implemented. I've left some suggestions inline to help improve the implementation.

})
}, [])

const setApiConfigurationField = useCallback(
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding JSDoc documentation for this function to explain the purpose of the isUserAction parameter. This would help future developers understand when to pass false. For example:

Suggested change
const setApiConfigurationField = useCallback(
/**
* Updates a field in the API configuration
* @param field - The field to update
* @param value - The new value
* @param isUserAction - Whether this change is from user interaction (true) or automatic initialization (false).
* User actions will mark the form as dirty and enable the save button.
*/
const setApiConfigurationField = useCallback(
<K extends keyof ProviderSettings>(field: K, value: ProviderSettings[K], isUserAction: boolean = true) => {

const isInitialSync = previousValue === undefined && value !== undefined
// Only skip change detection for automatic initialization (not user actions)
// This prevents the dirty state when the component initializes and auto-syncs values
const isInitialSync = !isUserAction && previousValue === undefined && value !== undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

The logic here is correct, but consider adding a comment to explain why we skip change detection for automatic initialization. This would help future maintainers understand the pattern.

if (!selectedModelId && !isInitialized.current) {
const initialValue = modelIds.includes(selectedModelId) ? selectedModelId : defaultModelId
setApiConfigurationField(modelIdKey, initialValue)
setApiConfigurationField(modelIdKey, initialValue, false) // false = automatic initialization
Copy link
Contributor

Choose a reason for hiding this comment

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

Good use of the comment here to clarify that this is automatic initialization! This pattern makes the intent very clear.

The useEffect that syncs apiModelId with selectedModelId was incorrectly
passing isUserAction=false, which prevented the save button from enabling
when users selected a different model. Since this effect responds to all
selectedModelId changes (including user selections), it should use the
default isUserAction=true behavior.
@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Aug 14, 2025
The test now correctly expects setApiConfigurationField to be called
with three arguments including the isUserAction=false parameter for
automatic thinking token adjustments.
@daniel-lxs daniel-lxs moved this from Triage to PR [Needs Review] in Roo Code Roadmap Aug 14, 2025
@hannesrudolph hannesrudolph added PR - Needs Review and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels Aug 14, 2025
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Aug 14, 2025
@mrubens mrubens merged commit bfdd158 into main Aug 14, 2025
16 checks passed
@mrubens mrubens deleted the fix/settings-save-button-provider-changes branch August 14, 2025 23:48
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Aug 14, 2025
@github-project-automation github-project-automation bot moved this from PR [Needs Review] to Done in Roo Code Roadmap Aug 14, 2025
fxcl added a commit to Anabaai/Roo-Cline that referenced this pull request Aug 16, 2025
* main: (70 commits)
  fix: use native Ollama API instead of OpenAI compatibility layer (RooCodeInc#7137)
  feat: add support for OpenAI gpt-5-chat-latest model (RooCodeInc#7058)
  Make enhance with task history default to true (RooCodeInc#7140)
  Bump cloud version to 0.16.0 (RooCodeInc#7135)
  Release: v1.51.0 (RooCodeInc#7130)
  Add an API for resuming tasks by ID (RooCodeInc#7122)
  Add support for task page event population (RooCodeInc#7117)
  fix: add type check before calling .match() on diffItem.content (RooCodeInc#6905) (RooCodeInc#6906)
  Fix: Enable save button for provider dropdown and checkbox changes (RooCodeInc#7113)
  fix: Use cline.cwd as primary source for workspace path in codebaseSearchTool (RooCodeInc#6902)
  Hotfix multiple folder workspace checkpoint (RooCodeInc#6903)
  fix: prevent XML entity decoding in diff tools (RooCodeInc#7107) (RooCodeInc#7108)
  Refactor task execution system: improve call stack management (RooCodeInc#7035)
  Changeset version bump (RooCodeInc#7104)
  feat(web): fill missing SEO-related values (RooCodeInc#7096)
  Update contributors list (RooCodeInc#6883)
  Release v3.25.15 (RooCodeInc#7103)
  fix: add /evals page to sitemap generation (RooCodeInc#7102)
  feat: implement sitemap generation in TypeScript and remove XML file (RooCodeInc#6206)
  fix: reset condensing state when switching tasks (RooCodeInc#6922)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working lgtm This PR has been approved by a maintainer PR - Needs Review size:M This PR changes 30-99 lines, ignoring generated files. UI/UX UI/UX related or focused

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants