nextcamp - fix flickering #5245
Conversation
|
this seems to remove the pre-caching though |
| <LoadingGoose | ||
| message={ | ||
| messages.length === 0 | ||
| messages.length === 0 && chatState === ChatState.Thinking |
There was a problem hiding this comment.
we should catch this all with chatState and then just have a mapping from chatState to message here I think
| handleSubmit(initialMessage); | ||
| } | ||
| }, [initialMessage, session, messages, handleSubmit]); | ||
| }, [initialMessage, session, handleSubmit]); |
There was a problem hiding this comment.
we just removed so much crufty hard to follow conditional code and now it is already coming back :(
this is trying to auto-submit initialMessage exactly once, but I don't think it works when switching sessions - hasSubmittedInitialMessage and initialMessagesLength never reset when sessionId changes. Even if we fixed that bug, the code is hard to follow with three separate refs, two effects with overlapping dependencies, and implicit coordination between them
I think we can make this work better if we move this into useChatStream; it can just decide after it has loaded the session from the server whether it should submit immediately and it is already scoped to the sessionId
| response: Response, | ||
| initialMessages: Message[], | ||
| setMessages: (messages: Message[], log: string) => void, | ||
| updateMessages: (messages: Message[]) => void, |
There was a problem hiding this comment.
any particular reason to rename this?
There was a problem hiding this comment.
just trying to avoid confusion and make it clear that its doing more than setMessages (setMessagesAndLog), really we're updating messages happy to change it back if you think that is confusing also
| setMessagesAndLog(session?.conversation || [], 'load-session'); | ||
| if (cancelled) return; | ||
|
|
||
| const sessionData = response.data; |
There was a problem hiding this comment.
why rename this? it is the session here
| setSessionLoadError(undefined); | ||
| setChatState(ChatState.Idle); | ||
| } | ||
| }, [sessionId, setMessagesAndLog]); |
There was a problem hiding this comment.
this useEffect has the same dependencies as the one below. I don't think there's a reason to have both. it also introduces a sessionIdRef that only checks if the sessionId has changed, but the sessionId is in the dependencies, so the useEffect already only runs when sessionId changes, no need for that ref.
I think all we want to do is add the resets to the block below
| if (error instanceof Error && error.name !== 'AbortError') { | ||
| log.error('submit failed', error); | ||
| onFinish('Submit error: ' + error.message); | ||
| } else if (error instanceof Error && error.name === 'AbortError') { |
There was a problem hiding this comment.
this can just be an else; but is also feels like duplication of what we're doing in stream from. so if we want to keep the exception catch here, we should remove the other one
| })(); | ||
|
|
||
| return () => { | ||
| cancelled = true; |
|
Unless I'm missing something I don't think the pre-caching was needed as we still have to wait for the server response and it was creating a race condition where the component had to coordinate between its own loaded state and useChatStream's state. Now we have a single source of truth in useChatStream and no flickering issues. Was the pre-cache intended to avoid rendering the existing chat messages when switching back to the chat? |
| return 'loading conversation...'; | ||
| } | ||
| return undefined; | ||
| }; |
There was a problem hiding this comment.
What I meant to say, but we can maybe do this in some other PR that we should move to a world where the useChatStream determines the chatState and BaseChat does not contain any logic. So isCompacting should just be a chatState, as should loading be. I checked and we are already doing this in the LoadingGoose component, so let's move the messaging we generate to one point
|
|
||
| const resultsCache = new Map<string, { messages: Message[]; session: Session }>(); | ||
| // Debug logging - set to false in production | ||
| const DEBUG_CHAT_STREAM = true; |
There was a problem hiding this comment.
we should delete this once we are sure it works really
| ); | ||
|
|
||
| useEffect(() => { | ||
| if (initialMessage && session && messages.length === 0 && chatState === ChatState.Idle) { |
There was a problem hiding this comment.
This is an improvement. I think we can do even better (maybe in a follow up) and make ChatState into a true statemachine that allows us to see exactly what's going on.
that way all we have to do here is depend on chatState and if it is autoSubmit and there is an initialMessage
Be careful deleting things that you don't know what they are for; Chesterton had his fence. so the goal of the precaching is that we have a version of the chat available immediately upon load when we hit a chat we've already worked with. in the current world switching back to a chat that you've worked with will reset the provider, model & extensions (in line with how it works now). this can be slow, so getting a copy immediately would be preferable. I'm not sure I am convinced this was the reason for the flickering though; the flickering only seemed to happen when we created a new conversation from the hub, in which cache there was no pre-cache by definition - the flickering happened during the streaming when we already have a session and the precache should be overwritten. there could be a bug in all of this of course, but I think this is the way to do it. |
Summary