715 defence successfully configured message should not disappear after a delay #778
Conversation
dhinrichs-scottlogic
changed the title
|
this change interacts amusingly with this bug #742
|
There was a problem hiding this comment.
I've not looked at the code yet, but this is a great change!
First thing I notice is that on refresh, or changing level then back again, these lovely new messages vanish! That's because when we add an info message to the messages state object, we must also update the chatHistory in the backend. See addInfoMessage defined in MainComponent.tsx.
Almost the same problem as #741
I'll do a proper review next week
Edit: Turns out it's only the model selection and configuration messages that disappear. I've found that addInfoMessage in MainComponenet.tsx will add the message to the front and as well as the backend history. So the fix is to use addInfoMessage rather than addChatMessage.
| } | ||
|
|
||
| async function setConfigurationValue( | ||
| defence: Defence, | ||
| configId: string, | ||
| value: string | ||
| ) { | ||
| const configIsValid = validateDefence(defence.id, configId, value); |
There was a problem hiding this comment.
I notice you've removed the validation. Do you plan to add it back in (here or in a fresh ticket) or do you think we don't need validation? It means that, for instance, you can configure things like this:
I went through and put invalid configurations and none of them actually stop the site from working, and the backend even comes back with somewhat sensible responses. If we decided to remove validation, then we should also remove validateDefence from defenceService.ts
There was a problem hiding this comment.
Yep I think that was a mistake when @dhinrichs-scottlogic pulled out the disappearing messages. We want to keep validation, and only call setDefenceConfiguration if we have a valid value.
That then leaves the question of what to show when the value is not valid; we would want an error message that persists beneath the input until the value is changed to a valid one, probably with error styling / validation state on the input to match.
That is not trivial, because we don't currently have validation on our themed inputs - ThemedNumberInput and ThemedTextArea.
Doro, I suggest you move the validation check into DefenceConfiguration, so we can have input-specific validation errors. We can put it in setDefenceConfigurationValueIfDifferent, and only call setDefenceConfiguration if the value is valid. We will want some local state to store whether the config input is valid or not, and will display some generic error-styled message underneath the DefenceConfigurationInput component when invalid.
As I anticipate some accessibility concerns, you could add a new issue to add accessible validation states to our themed inputs, unless you want to tackle that in this issue; in that case, you could do that part in a subsequent PR.
| }) { | ||
| const [value, setValue] = useState<number>(config.value); | ||
| const [showInfo, setShowInfo] = useState<boolean>(false); | ||
|
|
||
| async function handleValueChange(_: Event, value: number | number[]) { | ||
| const val = Array.isArray(value) ? value[0] : value; | ||
| setValue(val); | ||
| addChatMessage({ |
There was a problem hiding this comment.
You should import and use addInfoMessage instead. That'll sort out the refresh problem
| addChatMessage({ | ||
| type: CHAT_MESSAGE_TYPE.INFO, | ||
| message: `GPT model changed to: ${currentSelectedModel}`, | ||
| }); |
There was a problem hiding this comment.
You should import and use addInfoMessage instead. That'll sort out the refresh problem
There was a problem hiding this comment.
One primary concern echoing @pmarsh-scottlogic's comment about validation, but all else good.
I assume there are no tests covering these components? If you want to learn some Testing Library style component tests, you might consider adding some for one of the simpler components, or some around the proposed validation changes. Doesn't need to be in this PR though.
| } | ||
|
|
||
| async function setConfigurationValue( | ||
| defence: Defence, | ||
| configId: string, | ||
| value: string | ||
| ) { | ||
| const configIsValid = validateDefence(defence.id, configId, value); |
There was a problem hiding this comment.
Yep I think that was a mistake when @dhinrichs-scottlogic pulled out the disappearing messages. We want to keep validation, and only call setDefenceConfiguration if we have a valid value.
That then leaves the question of what to show when the value is not valid; we would want an error message that persists beneath the input until the value is changed to a valid one, probably with error styling / validation state on the input to match.
That is not trivial, because we don't currently have validation on our themed inputs - ThemedNumberInput and ThemedTextArea.
Doro, I suggest you move the validation check into DefenceConfiguration, so we can have input-specific validation errors. We can put it in setDefenceConfigurationValueIfDifferent, and only call setDefenceConfiguration if the value is valid. We will want some local state to store whether the config input is valid or not, and will display some generic error-styled message underneath the DefenceConfigurationInput component when invalid.
As I anticipate some accessibility concerns, you could add a new issue to add accessible validation states to our themed inputs, unless you want to tackle that in this issue; in that case, you could do that part in a subsequent PR.
| @@ -184,6 +184,9 @@ function MainComponent({ | |||
| return defence; | |||
| }); | |||
| setDefencesToShow(newDefences); | |||
| // add info message to chat | |||
| const displayedDefenceId = defenceId.replace(/_/g, ' ').toLowerCase(); | |||
| addInfoMessage(`${displayedDefenceId} defence reset`); | |||
There was a problem hiding this comment.
You have these two lines duplicated now; may as well pull out into a separate function, which takes param 'reset' or 'configured' for the message to add.
|
This all turned out much more complicated than anticipated, so after some discussion decided to approach it slightly differently and started again from scratch. |
chriswilty
deleted the
715-defence-successfully-configured-message-should-not-disappear-after-a-delay
branch
Description
Previously, when a user changed or reset the text or number inputs of the defence and model configurations, a message would pop up for 3 seconds to indicate to the user that the change has been made. This can easily be missed, which is why I moved this message to the chat, where the user already gets updates when defences get toggled on/off.
Additionally, I added an info message for the GPT model selection and the model sliders
Screenshots
Before:
After:
Notes
Concerns
Checklist
Have you done the following?