741 fix backend chat history message types #803
Conversation
…wn in the frontend
…used stuff from file
| chatMessageType, | ||
| infoMessage, | ||
| } | ||
| } as ChatMessage |
There was a problem hiding this comment.
This method allows invalidly typed chatMessages to be added to the history. Does it need to be fixed? Is it problematic if, because of client error, a message has superfluous properties?
There was a problem hiding this comment.
Hmm, good question. We might consider two improvements then:
- Create a type representing ChatMessages that originate from an API call (i.e. from the UI)
- Validate these messages on arrival, by checking their content conforms to our expected shape(s) and types.
Either or both of these could be done in a separate issue if you prefer, though you migth want to get them done now rather than kicking them down the road.
There was a problem hiding this comment.
Adding extra types seems not useful if types don't exist at runtime. We'd need to add explicit checks that would work even when the project is compiled to js, non?
There was a problem hiding this comment.
Okay, I've done my best shot at this, which may well could have been a separate PR's worth of work, so let me know if you want me to cherry pick commits across for ease of review.
- I've completely revamped the models/chatMessage.ts file which defines the chat message types. All the chat message types which will (in the history) store the message in the
infoMessageproperty are grouped together asCHAT_MESSAGE_TYPE_AS_INFO. This comes from an array of string literals. This same array is used to check that the message type in the request isn't bogus. - I realised that we were exclusively adding the messages as info messages in
handleAddToChatHistory. So really we should only accept chat message types that only store stuff in theinfoMessageproperty. So I renamedhandleAddToChatHistoryalong with the associated bits and bobs tohandleAddToChatHistoryAsInfo - While I was doing that I accidentally bumped into this bug and solved it before realising there was already a ticket for it. I added an extra unit test to catch future instances for good measure.
this is a type-safe as I can get it, but it's kinda ugly and the names are confusing, so I'm happy to try another approach / take suggestions.
…sh if response was blocked
backend/src/models/chatMessage.ts
Outdated
| 'INFO', | ||
| ] as const; | ||
|
|
||
| type CHAT_MESSAGE_TYPE_AS_INFO = (typeof chatMessageTypesAsInfo)[number]; |
There was a problem hiding this comment.
Can you explain the "as info" suffix to me? I can't see what you're trying to convey.
There was a problem hiding this comment.
Right. So as I was saying on teams, we have to things to distinguish.
"INFO"which is a specific type of chat message which should be rendered in a certain way on the front end. This is distinct from, but the same kind of thing as"LEVEL_INFO"and"LEVEL_RESET"etc.- chatMessages which store the the message in the
infoMessageproperty. I was assigning these the typechatMessageAsInfo.
This was weird and the names were confusing. So what I've done is
- Rename "INFO" to "GENERIC_INFO".
- Rename
chatMessageAsInfotochatInfoMessage.
And I've renamed the bits and bobs surrounding those too. The goal is to distinguish the two things. So now we have the idea of a chat info message, which is any message that appears in chat to give info, but doesn't get fed into the bot as a completion. Then we have the specific types that those messages can take, one of which is "GENERIC_INFO", and these types determine how they are rendered on the front end.
I think this is a satisfying resolution. What do you think?
There was a problem hiding this comment.
Yep I think this will work fine, and you'll likely be revisiting anyway when you look at consolidating network requests.
There was a problem hiding this comment.
"GENERIC_INFO"
Naming things is hard! How about "INFO_BUBBLE" or something like that? EVENT_INFO, INFORMATIVE, STATUS_UPDATE? Or just leave it as is cos you'll be revisiting this code anyway 😆
| chatMessageType: CHAT_MESSAGE_TYPE.USER, | ||
| infoMessage: message, | ||
| }) | ||
| ? chatHistoryWithNewUserMessages |
There was a problem hiding this comment.
I've lost track of this PR a touch! Looks like you're keeping the transformed message now even if it was blocked, so I'm assuming that's a deliberate change.
There was a problem hiding this comment.
Ah, it was not so deliberate. I want the new messages in the history, but I do not want the completion there. I shall change that now
There was a problem hiding this comment.
So now we need a version of type ChatUserTransformedMessage which doesn't include a completion. I was in two minds about whether I should just make the completion optional, or to make a whole new type that doesn't have the completion. I went with the former for lightweightness, but very happy to try the latter.
| function handleAddInfoToChatHistory( | ||
| req: OpenAiAddInfoToChatHistoryRequest, | ||
| res: Response | ||
| ) { |
There was a problem hiding this comment.
The more I think about this separate API call, the weirder it seems. In theory it allows us to inject an arbitrary message into the chat, of any chat message type. Ok we can't add a completion, but we could add a "BOT" message with an infoMessage property. Will the ChatGPT call simply ignore that message, as it has no completion property?
Anyhow, nothing to do here, just blurting out my thoughts as they form 😅
There was a problem hiding this comment.
well, not any more! They can only inject one that makes sense with the new checks I've added.
namely
if (... &&
chatInfoMessageType.includes(chatMessageType) &&
...
) {
// do the stuff
}
| }, | ||
| createAs === 'completion' | ||
| ? { | ||
| completion: { |
There was a problem hiding this comment.
Is indentation squiffy here? One too many indents?
There was a problem hiding this comment.
just Prettier doing its thing. Looks better in the ide
This PR is the result of a change request in #800 . It was deemed a large enough change to warrant a second PR. This branch came of the branch for the first PR. This PR should be merged after #800.
Description
Tightens up the types for the backend's chat message history. No functional change.
Notes
ChatHistoryMessagetoChatMessageCHAT_MESSAGE_TYPEs (enum). These specify whichCHAT_MESSAGE_TYPEs need completions and which need info messages, and lets us be more specific about our completion types.ChatMessageis now a union of the above types, and all of them live in their own new file, chatMessage.tsChecklist
Have you done the following?