Fix Cody Chat UI rendering reconciliation #5850
Conversation
|
|
There was a problem hiding this comment.
Tested locally and confirmed the states were not re-render.
I was looking into a similar issue for the editor state. Would appreciate your review on it as well since I think there are a few things I can clean up on my PR after yours is merged 😄 #5847
|
(Not a blocker) would be great to have a test covering transcript rendering for regression if possible 😄 |
vscode/webviews/utils/debugger.ts
Outdated
|
|
||
| useEffect(() => { | ||
| const changedProps = Object.entries(props).reduce( | ||
| (ps, [k, v]) => { |
There was a problem hiding this comment.
Our lints for PromptString rely on only using ps to refer to the PromptString formatter. This ensures we never alias ps, which can be used to defeat the static checks for correct PromptString usage.
We could write a more complicated check that understands JavaScript scoping to allow the use you have here, or only lint files which import "the" ps, but until we do that, can you rename these variables to anything else?
There was a problem hiding this comment.
Sure, renamed it in my last commit
There was a problem hiding this comment.
Approving for the ps fixes, thank you. I have not reviewed the rest in detail but I have some questions and suggestions inline.
I think the message output stuttering has a different cause, see #5296 (comment)
| @@ -0,0 +1,23 @@ | |||
| import { useEffect, useRef } from 'react' | |||
|
|
|||
| export function useTracePropsUpdate(props: any) { | |||
There was a problem hiding this comment.
Let's add a comment here that this is for debugging with advice about how to use it.
From time to team people do mass-deletions based on use. You probably intend for this to stick around.
| @@ -389,6 +389,17 @@ const TranscriptInteraction: FC<TranscriptInteractionProps> = memo(props => { | |||
| ) | |||
| }, isEqual) | |||
|
|
|||
| function useMutatedValue<T>(value?: T): [MutableRefObject<T | undefined>, setValue: (value: T) => void] { | |||
There was a problem hiding this comment.
You should document that this should not be used for rendering and won't cause re-rendering.
| @@ -180,7 +180,7 @@ const TranscriptInteraction: FC<TranscriptInteractionProps> = memo(props => { | |||
| smartApplyEnabled, | |||
| } = props | |||
|
|
|||
| const [intentResults, setIntentResults] = useState< | |||
| const [intentResults, setIntentResults] = useMutatedValue< | |||
There was a problem hiding this comment.
Why isn't simply useRef enough here?
Fixes part of https://linear.app/sourcegraph/issue/CODY-3943/optimize-performance-for-large-chat-windows-in-cody It looks like since sourcegraph#5036 we've lost memoization in a few places. As a result, our chat UI doesn't perform well with more than 4-5 messages (depending on how long these chat messages are) This PR tries to improve it by fixing memoization on expensive components (Transcript, HumanMessage, ...etc) | Before | After | | ------- | ------- | | <video src="https://github.com/user-attachments/assets/79e7b93c-ac82-4a8c-9b2e-6c712af3fb2c" /> | <video src="https://github.com/user-attachments/assets/223b3513-0f30-47cc-b24c-c54f4861329c" /> | This PR also fixes a problem with one-box intention detection; previously, it produced a state where we didn't have a follow-up message UI during intent resolution. ## Test plan - Check with the React profile that we don't render components during message response in places where we don't need to re-render anything - Check that intent detection in one-box doesn't produce UI flashes
Fixes part of https://linear.app/sourcegraph/issue/CODY-3943/optimize-performance-for-large-chat-windows-in-cody
It looks like since #5036 we've lost memoization in a few places. As a result, our chat UI doesn't perform well with more than 4-5 messages (depending on how long these chat messages are)
This PR tries to improve it by fixing memoization on expensive components (Transcript, HumanMessage, ...etc)
Screen.Recording.2024-10-08.at.20.41.09.mov
Screen.Recording.2024-10-08.at.20.39.49.mov
This PR also fixes a problem with one-box intention detection; previously, it produced a state where we didn't have a follow-up message UI during intent resolution.
Test plan