-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix: migrate provider settings #5205
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
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.
Caution
Changes requested ❌
Reviewed everything up to 392f110 in 1 minute and 57 seconds. Click for details.
- Reviewed
162
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
3
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/providers/DataProvider.tsx:47
- Draft comment:
Removal of migrateData() is intentional given the new migration in providers.ts. Verify that all migration logic is fully handled now. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to verify that all migration logic is fully handled, which violates the rule against asking the author to ensure behavior is intended or tested. It does not provide a specific suggestion or point out a specific issue.
2. web-app/src/services/providers.ts:15
- Draft comment:
Using localStorage as a migration flag works, but consider handling possible errors from getEngines (e.g. network or extension issues) to avoid silent failures. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The code does handle null/undefined cases through optional chaining. If getEngines() fails, engines will be undefined and the code handles that case. The migration flag in localStorage is used as a one-time check, so even if getEngines() fails, it will try again next time. This seems like a reasonable implementation pattern. I could be underestimating the impact of getEngines() failing. There might be important state that needs to be migrated that would be lost if we mark migration as complete without success. The code only marks migration complete after successfully getting engines data and processing it (line 69-71). If getEngines() fails, the migration flag won't be set, allowing retry on next load. The comment should be deleted. The code already handles failure cases appropriately through optional chaining and only marks migration complete after success.
3. web-app/src/services/providers.ts:29
- Draft comment:
Using key.replace('google_gemini', 'gemini') for provider name mapping may be brittle. Consider using a dedicated mapping function or configuration to handle potential future provider name changes. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The code is part of a migration process, which suggests this is a one-time conversion rather than an ongoing mapping need. The string replacement is simple and clear for this specific use case. The comment suggests overengineering a solution for what appears to be a temporary migration need. Looking at the rest of the file, we see similar direct mappings elsewhere (like 'cortex' to 'llama.cpp') which suggests this is the established pattern. The comment raises a valid point about maintainability. If more provider name mappings are needed in the future, having them scattered throughout the code could be problematic. However, this appears to be migration code that runs once, and the simple direct mapping is actually more readable and appropriate for this specific use case. Over-abstracting this would add unnecessary complexity. The comment should be deleted as it suggests over-engineering a simple, one-time migration mapping. The current implementation is appropriate for its purpose.
Workflow ID: wflow_yZ2OWLzo80AnG1Rm
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed 93a35e7 in 40 seconds. Click for details.
- Reviewed
23
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/services/providers.ts:75
- Draft comment:
Good change: migration flag is now set once after the mapping loop, avoiding redundant writes. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. web-app/src/services/providers.ts:66
- Draft comment:
Removing the migration flag set inside the map avoids multiple redundant writes. Verify that this single update meets the migration logic. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_kyLMqq04KymBvRqe
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Caution
Changes requested ❌
Reviewed 4173b85 in 29 minutes and 6 seconds. Click for details.
- Reviewed
14
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
0
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_3vMFy6i84jatP7uX
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
@@ -162,6 +162,9 @@ export default class JanInferenceCortexExtension extends LocalOAIEngine { | |||
}, | |||
}, | |||
]) | |||
this.updateCortexConfig({ |
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.
Consider awaiting the asynchronous call to updateCortexConfig
. Without await
, localStorage
marking migration as complete may occur before the config update finishes, potentially hiding backend update errors.
this.updateCortexConfig({ | |
await this.updateCortexConfig({ |
Describe Your Changes
Fixed an issue where users could not see provider settings on the new version when migrating to tauri.
Fixes Issues
Self Checklist
Important
Fixes provider settings migration to Tauri by integrating migration logic into
getProviders
and updating local storage.getProviders
inproviders.ts
.migration_completed
flag.migrateData
function fromDataProvider.tsx
andmigration.ts
.getProviders
inproviders.ts
to handle migration logic and update local storage.updateCortexConfig
call inindex.ts
to updatehuggingface_token
.migrateData
inDataProvider.tsx
.This description was created by
for 4173b85. You can customize this summary. It will automatically update as commits are pushed.