-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix: HuggingFace provider should be non-deletable #5856
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
fix: HuggingFace provider should be non-deletable #5856
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.
Important
Looks good to me! 👍
Reviewed everything up to 0d174db in 1 minute and 4 seconds. Click for details.
- Reviewed
27lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft 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/containers/dialogs/DeleteProvider.tsx:20
- Draft comment:
Importing predefinedProviders from '@/mock/data' replaces token.js models. Consider moving this config to a dedicated production config file if it's not meant only for mock/testing. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. web-app/src/containers/dialogs/DeleteProvider.tsx:31
- Draft comment:
The check using predefinedProviders.some() improves first-class provider detection. Consider extracting this logic into a helper function to enhance clarity and maintainability, and ensure the expected provider property exists in each predefined provider object. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_fSORwz0jQgnKQ0dI
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 bdc83b0 in 1 minute and 27 seconds. Click for details.
- Reviewed
61lines of code in4files - Skipped
0files when reviewing. - Skipped posting
3draft 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/containers/dialogs/DeleteProvider.tsx:31
- Draft comment:
Consider using a case‐insensitive comparison (e.g. using toLowerCase()) when checking if the provider is in predefinedProviders to be consistent with the provider creation check. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. web-app/src/routes/settings/providers/$providerName.tsx:369
- Draft comment:
Same as above – for consistency, consider using a case‐insensitive check when comparing provider names in predefinedProviders. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. web-app/src/routes/settings/providers/index.tsx:25
- Draft comment:
The import update from '@/mock/data' to '@/consts/providers' looks appropriate; ensure that the new constants match expected structure. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure that the new constants match the expected structure, which is a form of asking them to double-check their work. This violates the rule against asking the author to ensure behavior is intended or to double-check things.
Workflow ID: wflow_ntbCYFaCDM5ngjiZ
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 6acb12e in 1 minute and 59 seconds. Click for details.
- Reviewed
425lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft 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/__tests__/providers.test.ts:305
- Draft comment:
Mixing snake_case and camelCase keys (e.g. controller_type vs. controllerType) can be confusing. Consider standardizing the naming convention. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. web-app/src/services/__tests__/providers.test.ts:120
- Draft comment:
PR title mentions HuggingFace provider non-deletability but no test covers it. Add a test to verify that the HuggingFace (or similar predefined) provider cannot be deleted. - Reason this comment was not posted:
Comment was on unchanged code.
3. web-app/src/services/__tests__/providers.test.ts:18
- Draft comment:
Consistent use of trailing commas throughout mocks improves readability and diff clarity. Good job! - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_ewMKzgS4hkd6JqWp
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Barecheck - Code coverage reportTotal: 36.94%Your code coverage diff: 1.79% ▴ Uncovered files and lines
|
Describe Your Changes
This PR addresses feedback from #5808 regarding how we determine first-class provider status. Instead of using token.js registration as the check, we now implement a proper predefined list to identify first-class providers that should be non-deletable.
#5281
Self Checklist
Important
Refactor to use a predefined list for non-deletable providers, updating logic in
DeleteProvider.tsx,$providerName.tsx, andproviders.ts.DeleteProvider.tsx: UsespredefinedProvidersto check non-deletable status instead ofmodelsfromtoken.js.$providerName.tsx: Updates import path forpredefinedProviders.providers.ts: UsespredefinedProvidersfor built-in providers.providers.test.ts: Updates mock forpredefinedProvidersto reflect new import path and logic.web-app/src/mock/data.tstoweb-app/src/consts/providers.ts.providers.test.tsandproviders.ts.This description was created by
for 6acb12e. You can customize this summary. It will automatically update as commits are pushed.