-
Notifications
You must be signed in to change notification settings - Fork 2.3k
hotfix: regression issue with colon in model name #6008
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
hotfix: regression issue with colon in model name #6008
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 8eb8e71 in 1 minute and 34 seconds. Click for details.
- Reviewed
19lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft 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/hooks/useThreads.ts:47
- Draft comment:
Ensure removal of the ':free' check is intentional: previously, ids ending with ':free' were not split. Now splitting is based solely on provider. Confirm this change meets business requirements. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment is asking for verification of intention, which is explicitly against our rules. The code has a clear comment explaining the purpose of the change (Cortex migration). The change appears intentional and documented. Asking for business requirement confirmation is not appropriate for code review. Maybe the change in behavior could introduce bugs if some IDs with ':free' suffix are handled differently in other parts of the codebase? Even if there are potential issues elsewhere, that would be a cross-file concern which we're explicitly told to ignore. The change is clearly documented as part of a migration. Delete the comment as it violates our rules by asking for intention verification and business requirement confirmation. The change is clearly documented as part of a migration.
Workflow ID: wflow_cUmT25jID8dyTkwP
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 f86a4f9 in 44 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft 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/hooks/__tests__/useThreads.test.ts:90
- Draft comment:
Updated expected model id from 'thread1:free' to 'thread1/free' to match the new normalization. Ensure all parts of the codebase that reference model IDs follow this new format. - 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 all parts of the codebase follow the new format for model IDs. This falls under the rule of not asking the author to ensure behavior is intended or to double-check things. Therefore, this comment should be removed.
Workflow ID: wflow_fqc8SVWEH3y0G7Dn
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Describe Your Changes
Fixed the model selection issue where it auto switch back to unselected value when there is a colon in model name
Fixes Issues
Self Checklist
Important
Fixes model selection issue by replacing colons with slashes in model IDs for 'llama.cpp' in
useThreads.ts.useThreads.ts.useThreads.test.tsto expect model IDs with slashes instead of colons for 'llama.cpp' models.This description was created by
for f86a4f9. You can customize this summary. It will automatically update as commits are pushed.