refactor(prompts): Normalize playground instance messages #6218
Conversation
| 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
| 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?
| 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?
There was a problem hiding this comment.
We are accumulating messages for each initial instance. it is initialized as an empty object but accumulates every loop.
c454345 to
fca3202
Compare
* refactor(prompts): Normalize playground instance messages * Update use-zustand package to improve performance * Convert TemplateEditor, VariableEditor, and PlaygroundTool (editor) to uncontrolled components Workaround for uiwjs/react-codemirror#694 * Clean up variable names and semantics * Mark instance as dirty when updating message
* refactor(prompts): Normalize playground instance messages * Update use-zustand package to improve performance * Convert TemplateEditor, VariableEditor, and PlaygroundTool (editor) to uncontrolled components Workaround for uiwjs/react-codemirror#694 * Clean up variable names and semantics * Mark instance as dirty when updating message
* refactor(prompts): Normalize playground instance messages * Update use-zustand package to improve performance * Convert TemplateEditor, VariableEditor, and PlaygroundTool (editor) to uncontrolled components Workaround for uiwjs/react-codemirror#694 * Clean up variable names and semantics * Mark instance as dirty when updating message
Resolves: #6191
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.