828 streamline configure defence info message network call #874
Conversation
pmarsh-scottlogic
changed the title
| null | string, | ||
| | string | ||
| | { | ||
| resultingChatInfoMessage: ChatInfoMessage; |
There was a problem hiding this comment.
string or object - is that cos we have some error responses with a string body?
What do we do with those error responses? I'm now checking your changes in defenceService...
There was a problem hiding this comment.
Yes, sometimes we return an error message a simple string. Also yes, we swallow those. I suppose they remain helpful because you can see them in the browser inspector.
A note on these request types: This second part (ResBody) seems entirely redundant. TypeScript doesn't seem to enforce the types at all when you call res.send(...). Are we missing something?
There was a problem hiding this comment.
Yep I noticed that as well. I couldn't at the time work out what we were doing wrong, but now I look again, I think we should be accessing the response object from the request, i.e. req.res instead of passing a separate, untyped Response object to our handler functions.
| return response.status === 200; | ||
|
|
||
| const { resultingChatInfoMessage } = | ||
| (await response.json()) as ConfigureDefenceResponse; |
There was a problem hiding this comment.
This could be problematic because the response type in the case of errors is not application/json, it's text/plain. So I expect response.json() to fail in that case. Did you try it?
Better to check the status first, and if Ok then parse the JSON and make your DTO. And if it's not Ok, what happens to the error message? Looks like we are swallowing that currently.
Update Yep we are ignoring defence config errors when we could at least be writing something to the console, or better still, showing an error message somewhere.
There was a problem hiding this comment.
Yup, good call to check the status first. And yes, the message gets swallowed.
There was a problem hiding this comment.
not-swallowing the errors is a wider UI problem I reckon
There was a problem hiding this comment.
It's that "unhappy paths" issue come back to bite us 😁
pmarsh-scottlogic
deleted the
828-streamline-configure-chat-model-info-message-network-call
branch
⚠ Note, don't believe the branch name!
Description
Before, when the user would configure a defence, we would send a request to the backend. If the backend successfully configured the defence, then the frontend would send a further request to add the info message to the chat history, eg 'character limit defence updated'.
This PR changes it so that when the backend successfully configures a defence, the backend generates the info message and appends it to the history, then forwards it on to the frontend.
Concerns
Checklist
Have you done the following?