-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Goose Simple Compact UX #4202
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
base: main
Are you sure you want to change the base?
Goose Simple Compact UX #4202
Conversation
@alexhancock just a note to pull main and npm install, had to fix some dependencies from the upgrade |
7aa01ff
to
9c26e47
Compare
2675a40
to
6e4c5e6
Compare
6f4216f
to
f210830
Compare
23e8de7
to
dd2c7ea
Compare
@@ -242,7 +229,7 @@ function BaseChatContent({ | |||
const { createNewSessionIfNeeded } = useSessionContinuation({ | |||
chat, | |||
setChat, | |||
summarizedThread, | |||
summarizedThread: [], |
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.
@zanesq I wanted to ask you about this one. Can you clarify why the useSessionContinuation
hook needed the summarized thread before? Likely need to make some kind of change here before going ahead.
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 looks like this new architecture has replaced what useSessionContinuation
was doing so I think we can remove the hook completely now 👍
.bundle |
macOS ARM64 Desktop App (Apple Silicon)📱 Download macOS Desktop App (arm64, unsigned) Instructions: |
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.
Changes look nice overall; a little hard to parse the full diff on some of the file re-names. Taking it for a spin today though!
crates/goose/src/agents/context.rs
Outdated
The previous message contains a summary that was prepared because a context limit was reached. | ||
Do not mention that you read a summary or that conversation summarization occurred | ||
Just continue the conversation naturally based on the summarized context | ||
"); | ||
let assistant_message_tokens: usize = 14; |
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.
Bump to 41.
new_token_counts.insert(0, compaction_marker_tokens); | ||
|
||
// Add an assistant message to continue the conversation | ||
let assistant_message = Message::assistant().with_text(" |
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 have a second message here now? Is the idea that this will become agent visible only, and the other one will be user visible only?
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.
Is the idea that this will become agent visible only, and the other one will be user visible only
yes! to show the user a brief inline notification that compaction occurred, and give the model the deal of what happened and how it should behave when the session continues
|
||
// Store the original messages as ancestor messages so they can still be scrolled to | ||
if (setAncestorMessages) { | ||
const ancestorMessages = messages.map((msg) => ({ |
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.
Ancestor messages just live in memory in the client and don't survive reload?
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.
Reload seems to be having trouble now (filed #4255) but I think it would behave the same as reloading a conversation without compaction. Can check after 4255 is resolved
I noticed the new e2e tests aren't launching the app, are they working for you? Also I just pushed #4251 to main if you want to pull again so the pre-existing tests are passing. |
dd2c7ea
to
c2c690b
Compare
const continuationMessage = convertedMessages[2]; | ||
if (continuationMessage) { | ||
setTimeout(() => { | ||
append(continuationMessage); |
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.
TODO here - look into putting the original user message in if context ran out entirely
Noticed one issue where the green dot + context UI goes away right after a tool call. Comes back once goose returns control to the user. Auto-compact and button click flow seem pretty good. Having a hard time testing the out of context case; seems like GOOSE_CONTEXT_LIMIT doesn't actually override the context in desktop (works on CLI display but also doesn't seem like it properly forces context_limit_exceeded errors); might do a separate fix for that as this is super annoying to test otherwise, but trying to just get a big session file I can drop in with too. |
Seems like scrolling is a little flaky too; not sure if related at all to the branch. But the agent loop seems to only force the scrolling sometimes. In particular the first time scrolling after the first user input doesn't seem to work. |
Might be better with #4257 |
seems good so far @alexhancock - using it all morning and noothing surprising to me (not sure what I am quite looking for though). I did trigger a manual compact from GUI and did what I expected. Is there something I should look out for? Also been using it in cli |
I did get a token overrun though (cli) after a while:
which I have not seen in weeks, so not sure what is going on there. |
#4284 fixes the summarize button in the UI at the end of the context to your error. Saw some weird stuff rendered, but wasn't sure how to properly suppress that; not the end of the world though as we're targeting fixing this next with message metadata. (the top message is the summary which is also visible). I also don't see the context window display green dot after resuming a session (but before finishing processing on the first message). I think if we fix the green dot to stop disappearing this should be good to go. |
c2c690b
to
829edbe
Compare
4cf7136
to
c2661cd
Compare
c2661cd
to
df861f6
Compare
Updated demo screen capture UX.mov |
Improvements to the context management UX, making it much simpler and more consistent in the language it presents to users
Changes
Demo
Goose.Simple.Compact.mov