-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Unify loading goose messages and usechatstream determines chat state #5306
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
Conversation
|
Put into draft while working with upstream changes |
The ContextManager component was removed upstream in main. This commit: - Removes the import of useContextManager from useChatStream.ts - Removes the isCompacting hook usage - Returns chatState directly instead of wrapping it with effectiveChatState The Compacting state still exists in ChatState enum and is used by LoadingGoose, but the mechanism to set it will need to be implemented via system notifications or another approach if needed in the future.
Following the upstream pattern where ContextManager was replaced with system
notifications, this commit adds support for detecting compacting state:
- Added getCompactingMessage() helper to detect the compacting notification
message ('goose is compacting the conversation...')
- Updated useMessageStream to check for compacting messages and set
ChatState.Compacting when detected
- Compacting detection takes priority over thinking state detection
- This allows LoadingGoose to properly display the compacting UI
The backend sends a systemNotification with notificationType='thinkingMessage'
and msg='goose is compacting the conversation...' when compaction starts.
- Import getCompactingMessage and getThinkingMessage helpers - Add updateChatState parameter to streamFromResponse - Detect compacting/thinking messages in SSE stream - Prioritize Compacting state over Thinking state - Matches the implementation in useMessageStream
Instead of checking for exact string match 'goose is compacting the conversation...', now checks if the message contains 'compact' (case-insensitive). This is more flexible and won't break if: - The backend message wording changes slightly - Different variations are used (e.g., 'Compacting conversation') - The message is localized or customized Still maintains the check for thinkingMessage notification type to avoid false positives.
ui/desktop/src/types/message.ts
Outdated
|
|
||
| for (const content of message.content) { | ||
| if (content.type === 'systemNotification' && content.notificationType === 'thinkingMessage') { | ||
| if (content.msg.toLowerCase().includes('compact')) { |
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.
Wasn't sure if there was a better way here, could change it to look for the exact message goose is compacting the conversation... matching the backend?
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.
why is this of type thinking and not just of type compacting or some such?
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.
was wondering that also looks like we only have 2 SystemNotificationType from the backend 'thinkingMessage' | 'inlineMessage' didn't think it was worth refactoring the backend at this point but can follow up after
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.
ended up adding an exact match with the backend message I noticed we were doing that in other places
| const getLoadingMessage = () => { | ||
| if (message) return message; // Custom message takes priority | ||
|
|
||
| if (isLoadingConversation && chatState === ChatState.Thinking) { |
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.
can't we make Loading a chat state?
| if (chatState === ChatState.Thinking) return 'goose is thinking…'; | ||
| if (chatState === ChatState.Streaming) return 'goose is working on it…'; | ||
| if (chatState === ChatState.WaitingForUserInput) return 'goose is waiting…'; | ||
| if (chatState === ChatState.Compacting) return 'goose is compacting the 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.
this should be a look up table
| <FlyingBird className="flex-shrink-0" cycleInterval={150} /> | ||
| ) : chatState === ChatState.WaitingForUserInput ? ( | ||
| <AnimatedIcons className="flex-shrink-0" cycleInterval={600} variant="waiting" /> | ||
| ) : chatState === ChatState.Compacting ? ( |
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.
as should this
Instead of using a boolean prop 'isLoadingConversation' combined with ChatState.Thinking, we now have a dedicated ChatState.LoadingConversation. Changes: - Added LoadingConversation to ChatState enum - Updated LoadingGoose to use the new state and removed isLoadingConversation prop - Updated useChatStream to set LoadingConversation when loading a session - Updated BaseChat2 to remove isLoadingConversation prop usage Benefits: - Cleaner API - state is fully represented in ChatState - More explicit - LoadingConversation is distinct from Thinking - Easier to reason about - no need to check multiple conditions
* main: (77 commits) Fix legacy import (#5343) Unify loading goose messages and usechatstream determines chat state (#5306) Docs: goose session export and goose session import (#5267) Create recipe dir on save (#5337) docs: Update Discord link (#5335) [recipe workflow]: Fix `Invalid revision range` error (#5334) Add tech-article-explainer recipe (#5333) doc: added beta banner for old blog post (#5332) feat: add code refactor recipe (#5320) Security audit recipe (#5319) feat: add generate commit message recipe (#5326) fix: remove dependency on gsap library (#5330) feat: dynamically load ollama models (#5309) fix: skip temperature for goose-gpt-5 model (#5311) Replace compaction notifications with system notifications (#5218) Diagnostics (#5323) Fix gemini again (#5308) fix: synchronize local message state after conversation compaction (#5315) docs: replace broken links with working links (#5266) Add Web Accessibility Auditor recipe to cookbook (#5318) ...
…lock#5306) Co-authored-by: Douwe Osinga <douwe@squareup.com> Signed-off-by: Blair Allan <Blairallan@icloud.com>
* main: (54 commits) fixing typo in blog metadata (#5382) feat: add new blog entry on adopting Goose in the enterprise (#5381) Blog/acp intro oct 2024 (#5379) feat: add A/B test framework generator recipe (#5378) Doc: (blog) - Deep Dive into goose's Extension System and Model Context Protocol (MCP) (#5291) Some system prompt tidying (#5313) Fix scheduler jobs dates formatting (#5368) Use Instructions as Prompt in Scheduler (#5359) feat(snowflake): add support for newer Claude 4.5 and 4 models (#5350) Add bottom menu extension selection (#5352) (re)Standardize Session Name Attribute (#5279) chore: improve timeout for entering password when running goose ui from source (#5349) Fix legacy import (#5343) Unify loading goose messages and usechatstream determines chat state (#5306) Docs: goose session export and goose session import (#5267) Create recipe dir on save (#5337) docs: Update Discord link (#5335) [recipe workflow]: Fix `Invalid revision range` error (#5334) Add tech-article-explainer recipe (#5333) doc: added beta banner for old blog post (#5332) ...
…lock#5306) Co-authored-by: Douwe Osinga <douwe@squareup.com> Signed-off-by: Blair Allan <Blairallan@icloud.com>
Summary
Follow up for next camp to unify loading goose messages and usechatstream determines chat state