Fix defaultModel undefined error #5071
Conversation
|
@ZTH7 is attempting to deploy a commit to the NextChat Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe recent updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ModelManager
participant ModelTable
User->>ModelManager: Request to set default model
ModelManager->>ModelTable: Check if default model is valid
alt Valid default model
ModelManager->>ModelTable: Retrieve name and displayName
ModelManager->>User: Default model set successfully
else Invalid default model
ModelManager->>User: Error: Invalid default model
end
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/utils/model.ts (1 hunks)
Additional comments not posted (1)
app/utils/model.ts (1)
101-105: Enhanced validation fordefaultModel.The additional check ensures that
defaultModelexists inmodelTablebefore setting it as available and default. This change improves robustness by preventing invalid model IDs.
|
Your build has completed! |
app/utils/model.ts
Outdated
| @@ -98,10 +98,9 @@ export function collectModelTableWithDefaultModel( | |||
| defaultModel: string, | |||
| ) { | |||
| let modelTable = collectModelTable(models, customModels); | |||
| if (defaultModel && defaultModel !== "") { | |||
| if (defaultModel && defaultModel !== "" && defaultModel in modelTable) { | |||
There was a problem hiding this comment.
please see L82:
modelTable[`${customModelName}@${provider?.id}`] = {...}
There was a problem hiding this comment.
Pardon me, I don't understand your meaning. I know modelTable's key include the provider name, but the defaultModel should also specify the provider. Is there any problem here?
There was a problem hiding this comment.
我的意思是，当前系统已经是一个在线上运行了挺长时间，并且有很多人自己部署的系统。
所以，牵涉到配置相关的东西，需要尽量做到旧的配置能平稳迁移到新系统。
所以，这里可能需要做一下兼容（当用户旧的defaultModel没有配置provider name的时候，期望也能正常生效）
There was a problem hiding this comment.
translate:
I mean, the current system is one that has been running online for a long time and has been deployed by many people themselves.
Therefore, when it comes to configuration-related things, it is necessary to ensure that the old configuration can be smoothly migrated to the new system.
Therefore, some compatibility may be required here (when the user's old defaultModel does not have the provider name configured, it is expected to take effect normally).
There was a problem hiding this comment.
了解了，但鉴于用户旧的default_model没有指定provider，我选择创建一个独立的可选项，而不是帮他自动选择一个provider（如图）。
这同时也兼容了用户旧的default_model所指定的模型可能不在custom_models中的情况。
There was a problem hiding this comment.
或许针对就配置，帮用户选中匹配到的第一个才是合理的？
There was a problem hiding this comment.
但我们选到的第一个provider不一定可用，同时也会让这个函数变得繁琐，因为如果没有遍历到对应的模型，我们依旧需要create一个新element。
事实上我认为由用户来指定一个provider是必要的，如果用户没有指定，我们或许不应该随机选择。
当然如果你坚持，我将进行修改，毕竟两种方案在我看来都是可行的。
There was a problem hiding this comment.
- 确实，选到第一个不一定可用
但是，选到第一个可用的概率其实比较大
- 没有遍历到的时候，还是需要create新的element
这个逻辑，感觉不是很合理。应该是在available=true的模型中选择modelName=defaultModelName的第一个，标记一下isDefault=true
如果创建新的，感觉不合理，而且可能导致这个新创建的模型并不能正确的进行对话
There was a problem hiding this comment.
- 确实，选到第一个不一定可用
但是，选到第一个可用的概率其实比较大
- 没有遍历到的时候，还是需要create新的element
这个逻辑，感觉不是很合理。应该是在available=true的模型中选择modelName=defaultModelName的第一个，标记一下isDefault=true
如果创建新的，感觉不合理，而且可能导致这个新创建的模型并不能正确的进行对话
There was a problem hiding this comment.
是的，我也感觉创建一个新的不合理，但由于在之前的版本并没有检查defaultModel是否在modelTable中，因此我担心有用户的配置是靠这个"bug"生效的。
如果你觉得可以，我也打算如果找不到便不再创建新的，以免这个新创建的模型并不能正确的进行对话。
app/utils/model.ts
Outdated
| isDefault: true, | ||
| }; | ||
| const [modelName, providerName] = defaultModel.split("@"); | ||
| if (providerName && providerName != "") { |
There was a problem hiding this comment.
感觉还是不太严谨：
- 首先应该判断
defaultModel.includes('@') - 如果是新的配置方式，就判断
defaultModel in modelTable，如果在modelTable里面，就设置对应的这一项isDefault=true - 如果是旧的配置方式（实际上这个时候defaultModel就是modelName），走后面的for循环
- 在循环中，不应该使用
key.startsWith(modelName)，应该使用key.split('@').shift() == modelName
There was a problem hiding this comment.
感谢你的指点，关于
- 如果是新的配置方式，就判断
defaultModel in modelTable，如果在modelTable里面，就设置对应的这一项isDefault=true
这一点，是否可能存在用户使用了不在modelTable中的provider的情况？我是考虑到可能存在这一点，因此没写defaultModel in modelTable
There was a problem hiding this comment.
感谢你的指点，关于
- 如果是新的配置方式，就判断
defaultModel in modelTable，如果在modelTable里面，就设置对应的这一项isDefault=true这一点，是否可能存在用户使用了不在modelTable中的provider的情况？我是考虑到可能存在这一点，因此没写
defaultModel in modelTable
好像不会，我们不支持自定义provider，已更正。
There was a problem hiding this comment.
以后可能会有计划自定义provider
可能是一种alias的方式，例如都是OpenAI的模式，但是可以同时存在两个openai1 + openai2
但是还是一个构思中的处理方式，还没进入开发阶段
lloydzhou
added
help wanted
💻 变更类型 | Change Type
🔀 变更说明 | Description of Change
Fixed the issue where the variable Default_Model would produce "undefined" in the model list, and requests would return an "invalid model id" error after specifying the model provider in Default_Model.
📝 补充信息 | Additional Information
#5039
#5064
Summary by CodeRabbit