fix: allow isVisionModel function read runtime env var VISION_MODELS #5983
Conversation
|
@JiangYingjin is attempting to deploy a commit to the NextChat Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis pull request introduces a comprehensive enhancement to vision model handling across multiple components of the application. The changes primarily focus on expanding the configuration options for vision models by adding a new Changes
Sequence DiagramsequenceDiagram
participant Client as Client Application
participant AccessStore as Access Store
participant ConfigService as Config Service
participant ModelUtils as Model Utilities
Client->>AccessStore: Retrieve vision models
AccessStore-->>Client: Return vision models
Client->>ModelUtils: Check if model is vision model
ModelUtils->>ModelUtils: Combine default and custom vision models
ModelUtils-->>Client: Return vision model status
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
π§Ή Nitpick comments (2)
app/client/platforms/openai.ts (1)
197-198: Minor placement nitpick
DeclaringaccessStorea few lines above its usage might reduce clarity slightly. Consider moving these lines closer to whereisVisionModelis called for enhanced readability.app/components/chat.tsx (1)
1461-1462: Avoid duplicate store lookups
You're declaring anothercustomVisionModels = useAccessStore().visionModels;here, similar to earlier references. Consider consolidating the logic to a single place or passing the variable as a prop to reduce potential confusion and keep the code cleaner.
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (10)
app/api/config/route.ts(1 hunks)app/client/platforms/anthropic.ts(1 hunks)app/client/platforms/google.ts(1 hunks)app/client/platforms/openai.ts(2 hunks)app/client/platforms/tencent.ts(1 hunks)app/components/chat.tsx(5 hunks)app/config/server.ts(3 hunks)app/store/access.ts(1 hunks)app/utils.ts(2 hunks)test/vision-model-checker.test.ts(3 hunks)
π§° Additional context used
πͺ Biome (1.9.4)
test/vision-model-checker.test.ts
[error] 66-66: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
π Additional comments (25)
app/api/config/route.ts (1)
16-16: New property added for vision models.
The addition of visionModels: serverConfig.visionModels ensures that the vision model configuration is exposed through the DANGER_CONFIG. This is consistent with the newly introduced environment variable in server.ts. No issues detected.
test/vision-model-checker.test.ts (8)
5-5: Good approach to centralize custom vision models in test.
Defining customVisionModels in one place improves clarity and maintainability. Make sure to keep it in sync with any new additions.
31-31: Approved usage of the additional parameter.
Using customVisionModels as the second argument to isVisionModel properly tests custom models. This test is clear and robust.
36-36: Clear negative test case.
Verifying a non-vision model ensures the function correctly excludes them. This is good coverage.
48-48: Non-vision models tested thoroughly.
Similar to line 36, this test ensures correct exclusion handling for each model in the array.
54-54: Usage of runtime environment for vision models.
It's good that you're relying on process.env.VISION_MODELS dynamically, ensuring coverage for real-world scenarios.
55-59: Comprehensive checks for both environment-defined and custom models.
These lines verify that models from both sources are recognized or excluded correctly. Excellent test coverage.
64-64: Correct handling when VISION_MODELS is empty.
This covers a realistic edge case, ensuring the function safely defaults to no environment-defined vision models or relies on custom ones.
67-68: Verifying behavior after removing VISION_MODELS.
These checks confirm that the function behaves correctly when VISION_MODELS is no longer set. Good addition for realism.
app/store/access.ts (1)
126-126: New visionModels property introduced.
Adding visionModels to the default access state keeps the store consistent with the environment-driven configuration. No issues found.
app/config/server.ts (4)
24-24: VISION_MODELS environment variable declared.
This allows the runtime to define or override vision models dynamically, ensuring better configurability of vision features.
127-127: Safe fallback to empty string.
Using ?? "" as a fallback ensures that visionModels remains a string type. This straightforward approach prevents runtime errors if the env variable is undefined.
134-136: Applying logic within the GPT-4 disable block.
Filtering out 4-series models here is consistent. Ensure any future updates to these model prefixes also account for visionModels if needed.
254-254: Exposing visionModels in the config object.
This ensures that downstream consumers (e.g., DANGER_CONFIG) can access the vision model information. Good integration.
app/client/platforms/tencent.ts (1)
97-101: Looks good: usage of the new parameter
The newly introduced retrieval of accessStore.visionModels and passing it to isVisionModel correctly ensures runtime environment compatibility for vision models. No issues found with concurrency, error handling, or security.
app/client/platforms/google.ts (1)
86-86: Verify accessStore scope
Ensure that accessStore is declared in scope prior to its usage here. As long as your existing code properly defines it, this logic correctly utilizes isVisionModel with the new runtime vision models.
app/client/platforms/anthropic.ts (1)
89-93: Runtime environment reading is consistent
Accessing accessStore.visionModels to determine visionModel aligns with the new parameter approach. The code readability is good, and there are no evident logical or syntax issues.
app/client/platforms/openai.ts (1)
216-219: Correct integration of vision models
Passing accessStore.visionModels to isVisionModel ensures that externally set runtime environment variables are respected. This is a robust approach for supporting custom or user-provided vision models.
app/components/chat.tsx (5)
493-493: Variable type clarification
The customVisionModels variable references useAccessStore().visionModels. Ensure that this store value is consistently typed (e.g., string[] or string) throughout the codebase to avoid type mismatches when passing to other functions.
533-533: Check usage of isVisionModel arguments
Double-check that customVisionModels is a suitable type (string or string array) that matches the updated signature of isVisionModel. If visionModels is actually an array, consider converting it into a string or updating isVisionModel to accept an array parameter.
1466-1466: Check null/undefined safeguards
Before calling isVisionModel with customVisionModels, confirm that customVisionModels is defined to avoid any unexpected errors when joining strings inside isVisionModel.
1503-1503: Dependency array update
Adding customVisionModels to the dependency array ensures the callback re-runs when visionModels changes, which looks correct.
1551-1551: Minor comment addition
This line merely adds or updates a comment for shortcut keys without functional changes.
app/utils.ts (2)
10-10: Import usage confirmed
Importing getModelProvider here makes sense for checking model types. No issues found.
257-265: Validate string-manipulation logic for isVisionModel
The function signature now accepts customVisionModels as a string, then merges it with clientConfig?.visionModels using join, split, etc. This approach may lead to unexpected behavior if either variable is an array or if clientConfig?.visionModels is undefined. Consider:
- Double-check that both values are indeed strings.
- Implement extra checks to handle edge cases (e.g. empty strings, null, arrays).
- Potentially change the function parameter to
[string] | stringfor better clarity.
| }); | ||
|
|
||
| test("should handle empty or missing VISION_MODELS", () => { | ||
| process.env.VISION_MODELS = ""; | ||
| expect(isVisionModel("unrelated-model")).toBe(false); | ||
| expect(isVisionModel("unrelated-model", customVisionModels)).toBe(false); | ||
|
|
||
| delete process.env.VISION_MODELS; |
There was a problem hiding this comment.
π οΈ Refactor suggestion
Avoid using the delete operator.
Deleting environment variables can have performance overhead and may lead to unpredictable effects. Consider assigning process.env.VISION_MODELS = undefined instead of using delete.
- delete process.env.VISION_MODELS;
+ process.env.VISION_MODELS = undefined;π Committable suggestion
βΌοΈ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| delete process.env.VISION_MODELS; | |
| process.env.VISION_MODELS = undefined; |
π§° Tools
πͺ Biome (1.9.4)
[error] 66-66: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
π» εζ΄η±»ε | Change Type
π εζ΄θ―΄ζ | Description of Change
Problem: Before the fix, the
isVisionModelfunction readVISION_MODELSfrom thegetClientConfigfunction. However,getClientConfigwas unable to readVISION_MODELSfrom the program's runtime environment variables. This issue caused visual models outside the predefined and build-time environment variables to be evaluated asfalseby theisVisionModelfunction, resulting in them losing their visual capabilities. Specifically, theVISION_MODELSenvironment variable passed by users in Docker was not taking effect.Fix Logic: The
VISION_MODELSvariable was added toProcessEnvinapp/config/server.ts. This allows the client to read it via/api/configand store it inaccessStorealong with other variables likecustomModels. Since theisVisionModelfunction inapp/utils.tscannot directly accessaccessStore, a new parameter,customVisionModels, was added to theisVisionModelfunction. When callingisVisionModel,accessStore.visionModelsis passed to theisVisionModelfunction via the newcustomVisionModelsparameter. This enables theisVisionModelfunction to read theVISION_MODELSruntime environment variable.Fix Result: After the fix, the
isVisionModelfunction can readVISION_MODELSnot only fromgetClientConfigbut also from the program's runtime environment variables, resolving the problem described above.π θ‘₯ε δΏ‘ζ― | Additional Information
The test unit
test/vision-model-checker.test.tswas adjusted. Custom visual models were passed in through the newcustomVisionModelsparameter, and the tests passed.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests