refactor(prompts): Normalize playground instance messages #6218
Conversation
dosubot
bot
added
the
size:XL
| useEffect(() => { | ||
| if (readOnly) { | ||
| setValue(defaultValue); | ||
| } | ||
| }, [readOnly, defaultValue]); |
There was a problem hiding this comment.
Confused why this useEffect is needed. Isn't value defaulting to defaultValue anyways?
There was a problem hiding this comment.
it is defaulting to it, but never changes from it. This gives codemirror a stable "value" which it was already treating as a default value. Confusing.
There was a problem hiding this comment.
The useEffect makes sure to update the value with the new defaultValue if we are in readOnly mode. This lets codemirror blast away when new changes come in like normal, so that the caller doesn't have to mess with keys and remounting
| ); | ||
| const newMessages = arrayMove(messageIds, activeIndex, overIndex); |
There was a problem hiding this comment.
| const newMessages = arrayMove(messageIds, activeIndex, overIndex); | |
| const newMessageIds = arrayMove(messageIds, activeIndex, overIndex); |
| @@ -361,6 +352,17 @@ function SortableMessageItem({ | |||
| ChatMessage["toolCalls"] | |||
| >(message.toolCalls); | |||
|
|
|||
| const updateThisMessage = useCallback( | |||
There was a problem hiding this comment.
| const updateThisMessage = useCallback( | |
| const onMessageUpdate = useCallback( |
| useEffect(() => { | ||
| if (defaultValue == null) { | ||
| setInitialValue(""); | ||
| setVersion((prev) => prev + 1); | ||
| } | ||
| valueRef.current = defaultValue; | ||
| }, [defaultValue]); |
There was a problem hiding this comment.
is this useEffect needed? Seems un-necessary?
| valueRef.current = defaultValue; | ||
| }, [defaultValue]); | ||
| useEffect(() => { | ||
| setInitialValue(valueRef.current); |
There was a problem hiding this comment.
A bit confused why this is even entirely needed. Shouldn't we just not have initial values for this stuff?
| @@ -919,6 +927,12 @@ const getBaseChatCompletionInput = ({ | |||
| throw new Error("We only support chat templates for now"); | |||
| } | |||
|
|
|||
| const thisInstanceMessages = instance.template.messageIds | |||
There was a problem hiding this comment.
| const thisInstanceMessages = instance.template.messageIds | |
| const instanceMessages = instance.template.messageIds |
don't think you need this here
| if (instance.template.__type === "chat") { | ||
| const normalizedTemplate = normalizeChatTemplate(instance.template); | ||
| messageMap = { | ||
| ...messageMap, |
There was a problem hiding this comment.
why does messageMap need to be spread in here? It's largely an un-initialized object on line 190?
| let newMessageIds: number[] = []; | ||
| let newMessageMap: Record<number, ChatMessage> = {}; | ||
| if (firstInstance.template.__type === "chat") { | ||
| const existingMessageIds = firstInstance.template.messageIds; |
There was a problem hiding this comment.
| const existingMessageIds = firstInstance.template.messageIds; | |
| const messageIdsToCopy = firstInstance.template.messageIds; |
| ...message, | ||
| id: generateMessageId(), | ||
| })); | ||
| newMessageIds = existingMessages.map((message) => message.id); |
There was a problem hiding this comment.
| newMessageIds = existingMessages.map((message) => message.id); | |
| newMessageIds = newMessages.map((message) => message.id); |
| ...get().instanceMessages, | ||
| [messageId]: { | ||
| ...get().instanceMessages[messageId], |
There was a problem hiding this comment.
call get once before line 513 as the store? no need to call multiple times even if it might be technically inexpensive.
| const instance = state.instances.find( | ||
| (instance) => instance.id === instanceId | ||
| ); |
There was a problem hiding this comment.
can reUse selectPlayGroundInstance here.
| /** | ||
| * If not provided, a default empty message will be added | ||
| */ | ||
| messages?: ChatMessage | ChatMessage[]; |
There was a problem hiding this comment.
| messages?: ChatMessage | ChatMessage[]; | |
| messages?: ChatMessage[]; |
can we keep the type uniform?
This PR contains two major changes, and should be viewed by-commit.
Normalize playground instance messages
There is now a split in instance types:
The main difference is that chat templates within a normalized instance only contain messageIds instead of message objects.
The PlaygroundStore now only deals in PlaygroundNormalizedInstance, and handles conversion from PlaygroundInstance to PlaygroundNormalizedInstance during initialization.
Convert code mirror editors on playground to be uncontrolled
All of this is in service of fixing a react-codemirror bug that breaks rendering when updates take too long, which was occurring because messages were stored deeply within an instance causing a cascade of re-renders on every keystroke.