300 streamline frontend refresh network calls #836
Conversation
…uest which does the same thing as the health check
This reverts commit 6e29718.
|
@pmarsh-scottlogic Reviewing shortly, lunch first... |
| { level: LEVEL_NAMES.LEVEL_1, systemRole: systemRoleLevel1 }, | ||
| { level: LEVEL_NAMES.LEVEL_2, systemRole: systemRoleLevel2 }, | ||
| { level: LEVEL_NAMES.LEVEL_3, systemRole: systemRoleLevel3 }, | ||
| ]; |
There was a problem hiding this comment.
Hmm. We're sending the actual system roles for all levels, regardless of which level has been beaten? That doesn't feel right, even if that's what we were doing originally. Do we (or can we) send the systemRole for the level when we change level?
It's beginning to feel like we could use some persistent state in the UI, e.g. React Context. It's a bit late for all that now though 😢
There was a problem hiding this comment.
these system roles are for the handbook only. So technincally we only need to send the roles for levels that have been completed when this call happens. Then we'd need to send the next system role when numCompletedLevels increases. Which seems more complex than necessary in my view
There was a problem hiding this comment.
I don't think that's complex, as we are sending a payload back to the UI when the level is beaten anyway. It seems silly to hide the system role in the handbook, but it's right there in the network tab if you go looking.
| isMountRef.current = false; | ||
| }, []); | ||
| return isMountRef.current; | ||
| } |
There was a problem hiding this comment.
Feels a bit overkill to extract this into its own hook. All you're really doing is storing a ref to indicate when the component has mounted.
There was a problem hiding this comment.
I see no reason to not extract self-contained logic, even if it is trivial. Especially if un-extracting it will take extra effort. Do you think it is less readable this way?
There was a problem hiding this comment.
There are plenty of reasons not to extract self-contained logic, particularly into its own file. This one is used in just one component, which is my primary argument. It is no more readable sitting in its own file, than existing as a useRef in the same component that needs it.
Fragmenting your code is something to do when it aids in understanding and readability, because your code is becoming complex. But even in that case, there is rarely much benefit in extracting small pieces of code; instead you will benefit most from separating concerns.
I would like you to learn that refactoring can make things worse; keep related code co-located until such time as you find yourself repeating / copying logic. I am a strong advocate for keeping things simple; the Three Strikes rule is a good one to follow in cases like this:
https://en.wikipedia.org/wiki/Rule_of_three_(computer_programming)
There was a problem hiding this comment.
Interesting... This feels like something I'd have to learn the hard way to properly internalise it, but I'll take it on face value for now!
There was a problem hiding this comment.
I have learned the hard way ;)
In the back-end code you've done a whole heap of good separation-of-concerns type refactoring. Front-end code is a slightly different beast, and that also takes some getting used to. There's a pretty good argument made in the ReScript docs to keep project / component structure as flat as possible, co-locate related things, and generally not worry too much about splitting files, to avoid exactly this kind of debate!
There was a problem hiding this comment.
(Chris keeps quietly spreading the word about ReScript)
pmarsh-scottlogic
deleted the
300-streamline-frontend-refresh-network-calls
branch
* adds placeholder levelState endpoint * fill out handleGetLevelState * adds reminder comment * remove valid models and system roles from level state endpoint * add service to get levelSTate in frontend * adds leveStateService to index * corrects levelState url in frontend * further corrects levelState url in frontend * fixes typo in comment * renames levelState to handleStart * adds models and system roles back to start request * updates frontend service with changed name and added models and system roles * replace models and system roles front end calls with the start endpoint * erases all trace of system role endpoint * erases all trace of valid model endpoint * renames models to availableModels * removes helth check from frontend and adds catch to the handshake request which does the same thing as the health check * fix names from levelState to getStart in backend * only loads level info after first mount * separate logic for processing new level data from fetching it * remove some comments * moves main body key to App component * Revert "moves main body key to App component" This reverts commit 6e29718. * moves loadBackend data to maincoomponent * simplifies args to processBackendLevelData * converts chatMessageDTOs to chatMessages before returning from the service * moves messages state back to MainComponent * moves System roles state and openHandbook to mainComponent * removes redundant try catch and moves console log * puts startResponse in new models file combined.ts * removes comment * uses object destructuring to shorten declaration * renames to getValidOpenAIModels() * refactors loadBackendData * adds list of defences to show in level 3 by id * small rename * Revert "small rename" This reverts commit 60440bb. * Revert "adds list of defences to show in level 3 by id" This reverts commit a17d13b. * dinstinguishes between Defence and DefenceDTO. converts DTO to defence in the frontend service * fixes console log * remove defences_shown_level3 * clarifies nonModelDefences * renames ALL_DEFENCES to DEFAULT_DEFENCES * replaces a some with an includes * uses object destructuring to simplify startService * renames history to chatHistory in startResponse * fix mocking for getValidOpenAIModels * fixes different instance of same mocking problem * moves isFirstRender logic out of hook * sets isFirstRender at the the right place
Description
When you first load the website in the browser, or refresh we only send one network request which should grab all the necessary information to get going.
Notes
GET /startendpoint which is the single network call the frontend needs to set itself up. It returns the available models, the system roles for all levels and the emails, history and defences for the current level.GET /openai/validModelsendpoint and all related code (no longer needed)GET /systemRolesendpoint and all related code (no longer needed)loadBackendDataandopenHandbookto mainComponent.tsx so that all the/startfetched state stuff can be managed by one component.UseIsFirstRenderhook, for using with the useEffect hook. It indicates whether the useEffect is running on the first mount or on the change of a dependency.Checklist
Have you done the following?