-
Notifications
You must be signed in to change notification settings - Fork 616
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
draft: use rust messages in typescript #1393
Conversation
This is AI generated and needs some cleanup but is a start |
} | ||
|
||
#[derive(Debug, Deserialize)] |
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.
Great to see this diff with lots of red on the rust side!
const updatedMessages = await processMessageStream(response, requestMessages); | ||
|
||
// Auto-submit when all tool calls in the last assistant message have results | ||
if (maxSteps > 1 && updatedMessages.length > requestMessages.length) { |
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.
What is this part doing? Hard to grok
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.
This is incorrect I think, i will revisit
@@ -5,11 +5,8 @@ function SplashPill({ content, append, className = '', longForm = '' }) { | |||
<div | |||
className={`px-4 py-2 text-sm text-center text-textSubtle dark:text-textStandard cursor-pointer border border-borderSubtle hover:bg-bgSubtle rounded-full transition-all duration-150 ${className}`} | |||
onClick={async () => { | |||
const message = { | |||
content: longForm || content, | |||
role: 'user', |
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.
It doesn't look to me like the new append
implementation does anything to explicitly set that the message being appended is from the user https://github.com/block/goose/pull/1393/files#diff-b68456893042db8d6d236bc76aef8598d4ccabb69fb65a08158f2381d25492a2R336
Is there something I am missing that makes this work?
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.
yes later we define append as text => createUserMessage(text)
<MarkdownContent | ||
content={item.text} | ||
className="whitespace-pre-wrap p-2 max-w-full overflow-x-auto" | ||
/> | ||
)} | ||
{item.type === 'image' && item.data && item.mimeType && ( |
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.
Does this mean we drop images that could show up in ToolResult
s?
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.
yeah accidental here will restore this path
ui/desktop/src/utils/generateId.ts
Outdated
* Generate a random ID string | ||
* @returns A random string ID | ||
*/ | ||
export function generateId(): string { |
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.
This looks duplicated from the implementation in types/message.ts
- perhaps remove this file?
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.
We should also remove src/ai-sdk-fork/
which we can now!
No description provided.