Next camp remove useChatEngine #5734
Conversation
DOsinga
left a comment
DOsinga
left a comment
There was a problem hiding this comment.
happy to get this finally in, but I do think we should pay attention not to sneak back in the sins of old, like storing messages, extra useEffects etc
| return; | ||
| } | ||
|
|
||
| LocalMessageStorage.addMessage(userMessage.trim()); |
|
|
||
| type NotificationEvent = Extract<MessageEvent, { type: 'Notification' }>; | ||
| // JSON value type for notification params | ||
| type JsonValue = string | number | boolean | null | JsonValue[] | { [key: string]: JsonValue }; |
There was a problem hiding this comment.
we just painstakingly deleted this again, why are we bringing this back? redeclaring this as JsonValue over the unknown that the api generates, what does that get us? we could keep:
type NotificationEvent = Extract<MessageEvent, { type: 'Notification' }>;
but even there I am not sure we need it
There was a problem hiding this comment.
If I remove this and just use the base api type it has these typescript issues
src/components/ToolCallWithResponse.tsx:166:12 - error TS2339: Property 'data' does not exist on type '{}'.
166 params.data &&
~~~~
src/components/ToolCallWithResponse.tsx:167:19 - error TS2339: Property 'data' does not exist on type '{}'.
167 typeof params.data === 'object' &&
~~~~
src/components/ToolCallWithResponse.tsx:168:24 - error TS2339: Property 'data' does not exist on type '{}'.
168 'output' in params.data &&
~~~~
src/components/ToolCallWithResponse.tsx:169:24 - error TS2339: Property 'data' does not exist on type '{}'.
169 'stream' in params.data
~~~~
src/components/ToolCallWithResponse.tsx:171:23 - error TS2339: Property 'data' does not exist on type '{}'.
171 return `[${params.data.stream}] ${params.data.output}`;
~~~~
src/components/ToolCallWithResponse.tsx:171:46 - error TS2339: Property 'data' does not exist on type '{}'.
171 return `[${params.data.stream}] ${params.data.output}`;
~~~~
src/components/ToolCallWithResponse.tsx:174:17 - error TS18046: 'params' is of type 'unknown'.
174 return typeof params.data === 'string' ? params.data : JSON.stringify(params.data);
~~~~~~
src/components/ToolCallWithResponse.tsx:174:44 - error TS18046: 'params' is of type 'unknown'.
174 return typeof params.data === 'string' ? params.data : JSON.stringify(params.data);
~~~~~~
src/components/ToolCallWithResponse.tsx:174:73 - error TS18046: 'params' is of type 'unknown'.
174 return typeof params.data === 'string' ? params.data : JSON.stringify(params.data);
~~~~~~
Found 9 errors in the same file, starting at: src/components/ToolCallWithResponse.tsx:166
The auto-generated API types from the OpenAPI spec define the Notification event's message as:
{
message: {
[key: string]: unknown;
};
request_id: string;
type: 'Notification';
}
TypeScript sees params as unknown, so it can't access properties like params.data so we need to we need to refine the type to match the actual MCP notification structure.
The alternative is adding a bunch of type guards wherever this is used and that is way more messy.
Not sure why you think we don't need this, we want to define the type and make typescript happy. Can come back to it later when we refine the backend api types
There was a problem hiding this comment.
that is not defining the type it is changing it into anything goes. the code we're using it sometimes does the right thing like:
const notificationToProgress = (notification: NotificationEvent): Progress =>
notification.message.params as unknown as Progress;
that seems much better
There was a problem hiding this comment.
ah I understand what you are saying now :) ok updated in the parent branch since this is merged
| } | ||
| case 'Notification': { | ||
| updateNotifications(event); | ||
| updateNotifications(event as NotificationEvent); |
There was a problem hiding this comment.
we shouldn't need a cast here
There was a problem hiding this comment.
We still need as NotificationEvent when handling the event because TypeScript's discriminated union doesn't automatically narrow the type in the switch case - it still sees the broader MessageEvent type.
| console.error('Failed to stop power save blocker during cleanup:', error); | ||
| } | ||
| }; | ||
| }, [powerSaveTimeoutId]); |
There was a problem hiding this comment.
doesn't this immediately run when powerSaveTimeoutId gets set and therefore immediately clears the timer and the power saver blocker?
| setNotifications((prev) => [...prev, notification]); | ||
| }, []); | ||
|
|
||
| const stopPowerSaveBlocker = useCallback(() => { |
There was a problem hiding this comment.
this power save blocker stuff breaks multi chat. is the idea that we keep the computer awake but for at most 15 minutes? does that really help people?
if we want to keep this around and actually make it work, I think this needs to move to a separate thing where we keep track of all the sessions and all the time outs.
I'd be happy to ship without this for now too though
| import { useNavigation } from '../../hooks/useNavigation'; | ||
|
|
||
| // Helper function to determine if a message is a user message (same as useChatEngine) | ||
| // Helper function to determine if a message is a user message |
There was a problem hiding this comment.
if a comment starts lying, delete it. also though, we should just remove that from progressivemessagelist
…oose into zane/nextcamp-live-notifications-chatengine * 'zane/nextcamp-live-notifications' of github.com:block/goose: Next camp remove useChatEngine (#5734) # Conflicts: # ui/desktop/src/hooks/useChatStream.ts
Summary
Bring over missing power saver blocker and other functionality from useChatEngine and remove useChatEngine and related unused code
Bonus: Added missing click to view functionality to notification
Follow up for #5706