-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: prevent IO Intelligence model picker from reverting selection #7993
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
The ModelPicker component was incorrectly reinitializing the model selection when selectedModelId was falsy (empty string or undefined). This caused the IO Intelligence model picker to revert to the default model. Fixed by checking if the field is truly uninitialized (undefined) rather than just falsy, preventing unwanted reinitialization of already-set values. Fixes #7991
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 backward and I still missed the obvious bugs.
| const initialValue = modelIds.includes(selectedModelId) ? selectedModelId : defaultModelId | ||
| // Only initialize if the field has never been set (undefined), not when it's an empty string | ||
| if (apiConfiguration[modelIdKey] === undefined && !isInitialized.current) { | ||
| const initialValue = defaultModelId |
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.
I see we're removing the model validation logic here. The original code checked modelIds.includes(selectedModelId) to ensure only valid models were set. Without this check, we might initialize with an invalid model ID. Was this intentional, or should we preserve the validation while fixing the empty string issue?
| const initialValue = defaultModelId | |
| // Only initialize if the field has never been set (undefined), not when it's an empty string | |
| if (apiConfiguration[modelIdKey] === undefined && !isInitialized.current) { | |
| const initialValue = modelIds.includes(defaultModelId) ? defaultModelId : modelIds[0] || defaultModelId | |
| setApiConfigurationField(modelIdKey, initialValue, false) // false = automatic initialization |
|
|
||
| isInitialized.current = true | ||
| }, [modelIds, setApiConfigurationField, modelIdKey, selectedModelId, defaultModelId]) | ||
| }, [modelIds, setApiConfigurationField, modelIdKey, apiConfiguration, defaultModelId]) |
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.
By changing the dependency from selectedModelId to apiConfiguration, this effect now runs whenever ANY field in apiConfiguration changes, not just the model ID. This could trigger unnecessary re-initializations. Should we be more specific and use apiConfiguration[modelIdKey] instead?
| }, [modelIds, setApiConfigurationField, modelIdKey, apiConfiguration, defaultModelId]) | |
| }, [modelIds, setApiConfigurationField, modelIdKey, apiConfiguration[modelIdKey], defaultModelId]) |
| useEffect(() => { | ||
| if (!selectedModelId && !isInitialized.current) { | ||
| const initialValue = modelIds.includes(selectedModelId) ? selectedModelId : defaultModelId | ||
| // Only initialize if the field has never been set (undefined), not when it's an empty string |
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.
This comment could be more specific about why this distinction matters. Consider adding context about the IO Intelligence provider issue:
| // Only initialize if the field has never been set (undefined), not when it's an empty string | |
| // Only initialize if the field has never been set (undefined), not when it's an empty string | |
| // IO Intelligence can have empty string as a valid state, and we shouldn't reinitialize in that case |
|
I think it makes sense to address the review by Roomote, specially around the ´apiConfiguration´ dependency. |
Description
This PR attempts to address Issue #7991 where the IO Intelligence model picker was reverting to the previous selection when trying to change models.
Problem
The ModelPicker component was incorrectly reinitializing the model selection when
selectedModelIdwas falsy (empty string or undefined). This caused the IO Intelligence model picker to revert to the default model whenever a user tried to change the selection.Solution
Fixed the initialization logic to check if the field is truly uninitialized (
undefined) in the API configuration rather than just checking ifselectedModelIdis falsy. This prevents unwanted reinitialization of already-set values.Changes
ModelPicker.tsxto checkapiConfiguration[modelIdKey] === undefinedinstead of!selectedModelIdapiConfigurationinstead ofselectedModelIdTesting
Fixes #7991
Feedback and guidance are welcome!
Important
Fixes model selection reversion in
ModelPickerby checking forundefinedinitialization, ensuring correct model persistence.ModelPickerby checkingapiConfiguration[modelIdKey] === undefinedinstead of!selectedModelId.useEffectdependencies to includeapiConfigurationinstead ofselectedModelId.This description was created by
for ac5204f. You can customize this summary. It will automatically update as commits are pushed.