-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix: generate response button disappear on tool call #5988
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: generate response button disappear on tool call #5988
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 4da174a in 2 minutes and 34 seconds. Click for details.
- Reviewed
101
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
5
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/containers/dialogs/LoadModelErrorDialog.tsx:55
- Draft comment:
Changed error text from 'Failed to load model' to 'Something went wrong'. Consider whether a more specific message would aid debugging. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. web-app/src/hooks/useChat.ts:247
- Draft comment:
Conditionally adding the user message using the 'troubleshooting' flag prevents duplicate messages during regeneration. Confirm that this aligns with the intended behavior in regeneration flows. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. web-app/src/lib/messages.ts:41
- Draft comment:
Popping the previous user message to avoid consecutive user messages meets the Gemma3 requirement for user messages. Verify if a similar safeguard is needed for consecutive assistant messages. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment makes a speculative suggestion about assistant messages without evidence. It asks to "verify" something, which violates our rules. It references an external requirement ("Gemma3") that isn't visible in the context. The actual code change is clear and focused on user messages only. Maybe there really is a Gemma3 requirement about assistant messages that we don't know about. Maybe consecutive assistant messages could cause issues. Without clear evidence of the requirement or proof that consecutive assistant messages are problematic, we should not keep speculative comments or requests to verify things. The comment should be deleted as it asks for verification and makes speculative suggestions without evidence.
4. web-app/src/routes/threads/$threadId.tsx:223
- Draft comment:
The generateAIResponse function's new branch, which deletes non-user messages until a user message is found, appears to address incomplete tool call messages. Confirm that removing the user message before re-sending is the intended regeneration flow. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
5. web-app/src/routes/threads/$threadId.tsx:250
- Draft comment:
The condition for showing the 'Generate AI Response' button now also checks for tool_calls in the message metadata. Ensure that tool_calls are consistently set in the metadata (or equivalent) so that the button appears as expected in all tool call cases. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_xgl8LZlai43LAqpV
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 10763fd in 2 minutes and 26 seconds. Click for details.
- Reviewed
463
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
4
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/lib/__tests__/messages.test.ts:143
- Draft comment:
Consecutive user message de-duplication is correctly tested; the expectation now returns only the latter message as required by Gemma3. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, stating that a test is correctly implemented. It does not provide any actionable feedback or suggestions for improvement.
2. web-app/src/lib/__tests__/messages.test.ts:412
- Draft comment:
The integration test for a complex conversation flow comprehensively validates the correct ordering and handling of system, user, assistant (with and without tool calls), and tool messages. - 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/lib/__tests__/messages.test.ts:100
- Draft comment:
The behavior of converting messages with empty content to '.' is unexpected; consider adding a comment to clarify the intent behind this fallback. - Reason this comment was not posted:
Comment was on unchanged code.
4. web-app/src/lib/__tests__/messages.test.ts:336
- Draft comment:
The tests covering assistant message normalization with tags (including nested and multiple blocks) are thorough; the various edge cases are well validated. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, as it only states that the tests are thorough and edge cases are well validated. It does not provide any actionable feedback or suggestions for improvement.
Workflow ID: wflow_DcsUFYLJ7KWgqGya
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Barecheck - Code coverage reportTotal: 38.59%Your code coverage diff: -0.04% ▾ Uncovered files and lines
|
Describe Your Changes
This PR fixes 2 issues:
The generate button doesn't show as expected when there's an incomplete tool call message.
https://github.com/user-attachments/assets/c49973eb-2862-4833-82d1-8fc6baae727d
Gemma3 requires no consecutive user or assistant messages. This is to fix common cases such as regenerating and send a new message.
https://github.com/user-attachments/assets/3cbc869a-aab0-414e-ac48-5eed5c96cc6d
Fixes Issues
Self Checklist
Important
Fixes generate button visibility and prevents consecutive user messages in chat functionality.
useChat.ts
and$threadId.tsx
when there's an incomplete tool call message.CompletionMessagesBuilder
inmessages.ts
.addUserMessage()
inmessages.ts
to prevent consecutive user messages.generateAIResponse()
in$threadId.tsx
to handle tool call messages correctly.messages.test.ts
.LoadModelErrorDialog.tsx
to "Something went wrong".This description was created by
for 10763fd. You can customize this summary. It will automatically update as commits are pushed.