-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: enable Save button when clicking "Enable reasoning" checkbox #7101
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
…utton When the user clicks the "Enable reasoning" checkbox, the Save button should become enabled. Previously, the transition from undefined to true was treated as an initial sync, preventing the button from enabling. This fix explicitly excludes enableReasoningEffort from the initial sync logic since it is a user-controlled checkbox that should always trigger changes. Fixes #7099
|
This is not a robust solution, so I created a PR #7100 to fix it. Pls merge that PR first. |
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.
Reviewing my own code is like debugging in a mirror - everything looks backwards and I still can't find the problem.
| const isInitialSync = previousValue === undefined && value !== undefined | ||
| // Exception: enableReasoningEffort is a user-controlled checkbox that should always trigger changes | ||
| const isInitialSync = | ||
| previousValue === undefined && value !== undefined && field !== "enableReasoningEffort" |
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.
While this fix works for enableReasoningEffort, I found several other checkbox fields that might suffer from the same undefined → true transition issue:
anthropicBeta1MContextawsBedrock1MContextenableUrlContextenableGroundinglmStudioSpeculativeDecodingEnabledopenAiLegacyFormat
Should we investigate if these need the same treatment? Or perhaps consider a more scalable solution like maintaining a Set of user-controlled fields?
| // This prevents the dirty state when the component initializes and auto-syncs the model ID | ||
| const isInitialSync = previousValue === undefined && value !== undefined | ||
| // Exception: enableReasoningEffort is a user-controlled checkbox that should always trigger changes | ||
| const isInitialSync = |
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.
Is this approach intentional? Hard-coding field names here creates a maintenance burden. Could we consider using a more maintainable pattern like:
| const isInitialSync = | |
| // Don't treat initial sync from undefined to a defined value as a user change | |
| // This prevents the dirty state when the component initializes and auto-syncs the model ID | |
| // Exception: user-controlled checkboxes that should always trigger changes | |
| const USER_CONTROLLED_FIELDS = new Set(['enableReasoningEffort']) | |
| const isInitialSync = | |
| previousValue === undefined && value !== undefined && !USER_CONTROLLED_FIELDS.has(field) |
|
Fixed by #7113 |
Summary
This PR fixes an issue where the Save button in the Settings view does not become enabled when the user clicks the "Enable reasoning" checkbox.
Problem
When
enableReasoningEffortis initiallyundefinedand the user clicks the checkbox to enable it, the transition fromundefinedtotruewas incorrectly treated as an "initial sync" in thesetApiConfigurationFieldfunction. This prevented the Save button from being enabled because the change was not detected.Solution
The fix explicitly excludes
enableReasoningEffortfrom the initial sync logic since it is a user-controlled checkbox that should always trigger changes when toggled.Changes Made
webview-ui/src/components/settings/SettingsView.tsxto excludeenableReasoningEffortfrom the initial sync conditionTesting
Fixes #7099
Important
Fixes issue in
SettingsView.tsxwhere the Save button does not enable when the "Enable reasoning" checkbox is clicked by excluding it from initial sync logic.enableReasoningEffortfrom initial sync logic insetApiConfigurationField()inSettingsView.tsx.This description was created by
for 5d8e667. You can customize this summary. It will automatically update as commits are pushed.