Skip to content
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.

Sign up for GitHub

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

fix: default model #5553

Merged
merged 1 commit into from
Sep 27, 2024
+7 βˆ’4
Merged

fix: default model #5553

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions app/store/access.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,10 +211,13 @@ export const useAccessStore = createPersistStore(
})
.then((res) => res.json())
.then((res) => {
// Set default model from env request
let defaultModel = res.defaultModel ?? "";
if (defaultModel !== "")
DEFAULT_CONFIG.modelConfig.model = defaultModel;
const defaultModel = res.defaultModel ?? "";
if (defaultModel !== "") {
const [model, providerName] = defaultModel.split("@");
DEFAULT_CONFIG.modelConfig.model = model;
DEFAULT_CONFIG.modelConfig.providerName = providerName;
}
Comment on lines +214 to +219
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

πŸ’‘ Codebase verification

Potential Impact on app/store/config.ts

The modification of DEFAULT_CONFIG.modelConfig in app/store/access.ts also affects app/store/config.ts. Please review the usage in config.ts to ensure consistency and prevent unintended side effects.

  • app/store/config.ts: DEFAULT_CONFIG.modelConfig.compressModel
  • app/store/config.ts: DEFAULT_CONFIG.modelConfig.compressProviderName
πŸ”— Analysis chain

Approve with suggestions: Improve error handling and verify integration

The changes address the PR objective of fixing the default model by allowing more granular control over the model and provider. However, there are a few points to consider:

  1. Error Handling: The code assumes that defaultModel always contains the "@" character. Consider adding error handling for cases where the split operation fails.

  2. Server Response Format: Verify that the server always sends the defaultModel in the expected format (model@provider). If not, additional checks may be needed.

  3. Integration: This change might affect other parts of the application that rely on DEFAULT_CONFIG.modelConfig. Please ensure that all dependent code is updated accordingly.

To verify the integration and usage of DEFAULT_CONFIG.modelConfig, you can run the following script:

This script will help identify areas of the codebase that might be affected by the changes to DEFAULT_CONFIG.modelConfig.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check usage of DEFAULT_CONFIG.modelConfig

# Search for usage of DEFAULT_CONFIG.modelConfig
echo "Searching for DEFAULT_CONFIG.modelConfig usage:"
rg --type typescript --type javascript 'DEFAULT_CONFIG\.modelConfig'

# Search for imports of DEFAULT_CONFIG
echo "\nSearching for DEFAULT_CONFIG imports:"
rg --type typescript --type javascript 'import.*DEFAULT_CONFIG'

Length of output: 400

Script:

#!/bin/bash
# Description: Check usage of DEFAULT_CONFIG.modelConfig in .ts and .js files

echo "Searching for DEFAULT_CONFIG.modelConfig usage in .ts and .js files:"
rg 'DEFAULT_CONFIG\.modelConfig' --glob '*.ts' --glob '*.js'

echo -e "\nSearching for imports of DEFAULT_CONFIG in .ts and .js files:"
rg 'import.*DEFAULT_CONFIG' --glob '*.ts' --glob '*.js'

Length of output: 782


return res;
})
.then((res: DangerConfig) => {
Expand Down
Loading