Introducing the ability to remove specified Providers. #5992
Conversation
|
@QAbot-zh is attempting to deploy a commit to the NextChat Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request introduces comprehensive updates to the NextChat application's README documentation and model management utility. The changes focus on expanding environment variable configurations, providing more detailed instructions for model deployment, and enhancing the flexibility of model selection. The README now includes extensive documentation for various API keys, model customization options, and deployment settings across multiple providers like OpenAI, Azure, Google, Anthropic, and others. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
既然改到这个地方了，如果方便可以针对此函数加一个单元测试吗 |
Now that we have changed it to this place, if it is convenient, can we add a unit test for this function? |
![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
🧹 Nitpick comments (2)
test/model.test.ts (2)
79-79: Correct the minor typo in the test description.The word "shoule" should be changed to "should" for accuracy.
- test('all models shoule be available', () => { + test('all models should be available', () => {
56-56: Remove the empty array slot to avoid sparse arrays.Line 56 has a trailing comma that introduces an empty slot in
mockModels. Although JavaScript allows sparse arrays, it can lead to unintended behavior. Consider removing the trailing comma or using a placeholder value (undefinedor similar) intentionally.56- , + // Remove or replace with a placeholder if needed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/model.test.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
test/model.test.ts
[error] 5-77: This array contains an empty slot.
Unsafe fix: Replace hole with undefined
(lint/suspicious/noSparseArray)
已添加该函数的单元测试 |
A unit test for the function has been added |
Some tests failed |
|
Your build has completed! |
|
模型名称抄错了，已更正 |
The model name was copied incorrectly and has been corrected. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/model.test.ts (2)
78-78: Fix typo in test descriptionChange "shoule" to "should" in the test description.
- test('all models shoule be available', () => { + test('should have all models available', () => {
129-141: Update test description and add validation for invalid patterns
- The test description doesn't match the implementation as it includes an Azure model (
gpt-4o@azure).- Consider adding tests for invalid filter patterns.
- test('should exclude all models except OpenAI when custom is "-all, +*openai"', () => { + test('should exclude all models except OpenAI and specific Azure models when custom is "-all, +*openai, gpt-4o@azure"', () => {Add a new test for invalid patterns:
test('should handle invalid filter patterns gracefully', () => { const invalidPatterns = [ '*invalid*pattern', 'missing-operator*azure', '-incomplete+' ]; invalidPatterns.forEach(pattern => { const result = collectModelTable(mockModels, pattern); // All models should remain available with invalid patterns expect(result['gpt-3.5-turbo@openai'].available).toBe(true); expect(result['gpt-4@openai'].available).toBe(true); // ... add assertions for other models }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/model.test.ts(1 hunks)
🔇 Additional comments (1)
test/model.test.ts (1)
4-142: Well-structured test suite with good coverage
The test suite effectively validates the provider filtering functionality with comprehensive coverage of different scenarios. It includes:
- Default behavior
- Complete exclusion
- Provider-specific exclusion
- Multiple provider exclusion
- Complex filter combinations
This addresses the PR objectives and provides good test coverage for the core functionality.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
test/model.test.ts (3)
1-76: Consider improving mock data organization and type safety.The mock data setup is comprehensive, but consider these improvements:
- Move the
mockModelsarray to a separate test fixture file for reusability across test files- Replace
as LLMModelProvidertype assertions with proper object creation- } as LLMModelProvider, + }: LLMModelProvider = { + id: 'openai', + providerName: 'OpenAI', + providerType: 'openai', + },
78-89: Fix typo in test name.There's a spelling error in the test name: "shoule" should be "should"
- test('all models shoule be available', () => { + test('all models should be available', () => {
103-127: Consider using test.each for provider filter tests.These provider-specific filter tests follow a similar pattern. Consider using
test.eachto reduce code duplication and make it easier to add more provider filter test cases.test.each([ { name: 'should exclude all Azure models', filter: '-*azure', expected: { 'gpt-3.5-turbo@openai': true, 'gpt-3.5-turbo@azure': false, // ... other expectations } }, { name: 'should exclude Google and XAI models', filter: '-*google,-*xai', expected: { 'gpt-3.5-turbo@openai': true, 'gemini-pro@google': false, // ... other expectations } } ])('$name when custom is "$filter"', ({ filter, expected }) => { const result = collectModelTable(mockModels, filter); Object.entries(expected).forEach(([model, available]) => { expect(result[model].available).toBe(available); }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/model.test.ts(1 hunks)
🔇 Additional comments (1)
test/model.test.ts (1)
90-101: LGTM! Comprehensive test for the "-all" filter.
The test thoroughly verifies that all models are disabled when using the "-all" filter.
|
|
|
|
（可算是摸通测试的流程了） 原先测试文件里的 custom_models 里多写了空格，已更正，yarn test 通过截图
|
(It can be considered as a thorough understanding of the testing process) There were extra spaces in custom_models in the original test file, which has been corrected. Yarn test passed the screenshot.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/model.test.ts (3)
1-76: LGTM! Consider using type construction instead of type assertionThe mock data setup is well-structured with comprehensive test cases. However, there's a minor improvement opportunity in the type assertions.
Instead of using type assertion with
as LLMModelProvider, consider constructing the type directly:- } as LLMModelProvider, + }: LLMModelProvider = {This approach is generally safer as it ensures all required properties are provided at compile time.
78-78: Fix typo in test descriptionThere's a typo in the first test description.
- test('all models shoule be available', () => { + test('all models should be available', () => {
78-141: Consider adding tests for filter string edge casesWhile the current tests cover the main functionality well, consider adding tests for filter string edge cases:
- Whitespace handling (e.g.,
" -all , +*openai ")- Case sensitivity (e.g.,
"-ALL"vs"-all")- Duplicate filters (e.g.,
"-all,-all")- Empty filter segments (e.g.,
",,,-all,,,")Example test case:
test('should handle whitespace in filter string correctly', () => { const customModels = ' -all , +*openai '; const result = collectModelTable(mockModels, customModels); expect(result['gpt-3.5-turbo@openai'].available).toBe(true); expect(result['gpt-4@openai'].available).toBe(true); expect(result['gpt-3.5-turbo@azure'].available).toBe(false); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/utils/model.ts(2 hunks)test/model.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/utils/model.ts
🧰 Additional context used
📓 Learnings (1)
test/model.test.ts (1)
Learnt from: QAbot-zh
PR: ChatGPTNextWeb/ChatGPT-Next-Web#5992
File: test/model.test.ts:129-141
Timestamp: 2024-12-28T15:48:43.321Z
Learning: The test logic allows adding custom models not present in the mockModels array by using the customModels filter. This is intentional behavior to verify that customModels can introduce new models beyond the predefined list.
🔇 Additional comments (1)
test/model.test.ts (1)
78-141: LGTM! Test cases provide good coverage of core functionality
The test suite effectively covers the main scenarios for model filtering and availability. The test cases are well-structured and clearly named, making the expected behavior obvious.
|
@Dogtiti 同样的问题，失败的是另外的测试文件
|
@Dogtiti Same problem, the failure is another test file
|
💻 变更类型 | Change Type
🔀 变更说明 | Description of Change
解决大家长期困扰的问题，支持通过 - 指令移除不需要的 provider，尤其是 openai，azure 模型列表共生带来的困扰
#5988
#5050
📝 补充信息 | Additional Information
Summary by CodeRabbit
Documentation
New Features
Tests