Resolve "[MIN-113] Show comments on the map"

changed milestone to %v18.0.0~alpha.0

added 1 deleted label

added 8 commits

a63b3880 - layout for comment button
a1767c94 - comment schema
2e1e8ce1 - commentFeature
4d971ccb - reducer for comments
9a0b09a4 - comment selector implemented
122b61c5 - show comments
8b5ac299 - toggle comments is working
c3ea55a3 - change button text on show/hide

Compare with previous version

marked this merge request as ready

Hey Piotr, I think this MR is fine. I'll give you my thoughts on it and some pointers.

State

You could use more useState and less redux. Even if the state is shared several levels deep, it's usually fine. There's less than 40 useStates in the code while if I search for reducer I get around 600 occurrences. It's a bit like using a shotgun to kill flies.

Tests

How much overlap do these tests have with manual tests? Some tests seem to be just writing a bunch of code to replicate what the browser does. But this is sort of what cypress and playwright do for you automatically and with support for multiple browsers. You can take a look at the example here: https://playwright.dev/docs/codegen

React

This is a good site to understand the hooks rules: https://react.dev/reference/rules/rules-of-hooks

I'd say one simplified way to understand the hooks rules is that:

Each component is a function
This function runs on each re-render
The hooks use<Something> keep track of the state but only if they are executed every time this component function is executed. You put an if and it won't be able to track the state

A nice site to learn how the renders work:

https://alexsidorenko.com/blog/react-render-always-rerenders/

I think you could probably simplify stuff like this on MapNavigation.component.tsx:

 const isActive = (modelId: number): boolean => currentModelId === modelId;

// some code here and after inside JSX you call this multiple times:
isActive(map.modelId)

you could replace this with

const isActive === map.modelId === currentModelId;

and just use the isActive variable on the JSX.

How it is now is fine, this is just an example. Because React is a bit like a game engine, and the components run every time the main game loop thinks it's time to render an update. Vue doesn't do this for instance.

Sonarqube

Can you see if you have access to this sonarqube url? https://sonarqube.lcsb.uni.lu/dashboard?id=minerva_frontend_AZArgG_56FdDjrKOmf_E

The issues are fine, the security hotspots seem to be ok. But if you'd like we could take a look at it. You could probably clean up this in a couple of days. The issues are almost all the same.

You can add this to the pipeline if you think the feedback makes sense. There's an howto: https://howto.lcsb.uni.lu/publication/code-review/

General

The .types.ts is not a standard. You could export it from the reducer, I notice I do a lot of jumping around between files with less than 10 lines.
When I open the console I see a bunch of errors. It seems to be mainly downshift. Usually I like to get rid of these because sometimes they are warnings for subtle bugs and it's hard to notice if new errors are appearing.

Conclusion

I'd probably reduce on the unit tests and try to leverage the human testers. And then, automate some parts with cypress or playwright.

Redux/RTK with redux-thunk is quite complex and it sort of takes a hold of the codebase. I was just jumping around to see "oh this toggles a boolean". I'd try to reduce its use.

I like this comment: "Don't use redux, unless you fully understand how it works, then you will realize that you should definitely not use it." :)

It looks like currently you must be losing a bunch of time with RTK and tests and at the end of the day, they don't do much more than the testers and you just lose time.

The new developers will understand your code for sure. Frontend for complex interactions is hard. Even so much better than the jquery + IE6 days. I sort of got used to the footguns and just suck it up

Hope this helps!

resolved all threads

merged

mentioned in commit 79be1a4f