hpi-sam / digital-fuesim-manv Goto Github PK

There should be a route to create an exercise.

The client should be removed from the state after its socket disconnects from the backend. This includes sending the appropriate action to all clients of the exercise.

Requirements

Synchronisation between server/clients

• a client can subscribe to a part of the state (example: all vehicles and personnel in viewport1 (= calculated via coordinates))
• state should be robustly stored (backups) and handled (= database)
• the state change can be executed without conflicts (e.g. a client changing the position of a vehicle during the state-update)
• state changes should be validated by a trusted entity
• a client gets updates to the state in real-time (e.g. "push" like websockets, no polling)

(local) State management client

• in the client one should be able to subscribe to a part of the state (async observer pattern) and get snapshotted values directly in a synchronous way
• nicely encapsulated client-side optimistic updates for more responsive ux

both

• a client can modify the data and send the change to the server/the other clients
• very good typings and reusability across all codebases (client and server)

TODOs

• API design websockets
• senden des States über websocket verbindung
• backend ordentlich machen (file structure, watch mode)
• action validation (@ClFeSc)
• setup test frameworks
  • backend
  • shared
  • frontend unit tests
  • e2e
• optimistic updates
• Add actionNr to performAction and getState to fix possible synchronisation issues
• Resolve all TODOs in the current code base

libraries to investigate

realtime database

• firebase realtime database
• SapphireDb
• supabase
• roomservice
• replicache

client-side state management (stores)

• NGXS
• NGRX

We currently have some vulnerabilities in the frontend (run npm audit in the frontend to see them). We should

• mitigate them by updating the affected packages, and
• add auditing to the CI

in order to not get any bigger problems from them.

• TODO: evaluate wehter something like this makes sense: https://www.youtube.com/watch?v=9iU_IE6vnJ8

As the title says, dependabot is currently set up to use labels that a) do not exist and b) are invalid label names.

There should be an overview for all clients that is only visible to trainer. It should include a list of all clients, their roles and display names. Adding/removing a client to/from the waiting room should also be possible (if the waiting room already exists).

Jest starts the backend without us wanting it to. This required us to write some ugly singleton stuff in #47. We should look into preventing Jest from doing such things.

There should be a route, e.g. GET /join/:exerciseId that automatically (after asking for the display name) joins a client to the exercise provided by :exerciseId.

Introduce the strict use of readonly into our codebase.

• add Immutable, Mutable helpers and use them for the actions and state types as well as function parameters like in the reducers and the optimisticActionHandler.

• I'm not sure about their use as interfaces yet, but that's easy to try out.

• Maybe we could/should also introduce a JSON or PlainObject type (no Map/functions ...).

This is a collection for all issues/TODOs/discussions relating the CI.
After the heavy lifting is done this can be closed and remaining (longer standing/less important) TODOs get their own issue.

TODOs

1. Reuse build artifacts
2. Is it even necessary to lint on windows and macos in addition to linux?

• deployment etc.

We currently pass the root .gitignore as the ignore file for prettier.

Prettier doesn't respect nested .gitignore files.

My proposal: we merge all our .gitignore files into the root file.

The tests still fail. I can only imagine that this is due to invalid caches. Maybe caching shared doesn't work as expected.

Store the info about processed files in order to only operate on the changed ones. The cache is stored in .eslintcache by default. Enabling this option can dramatically improve ESLint's running time by ensuring that only changed files are linted.
https://eslint.org/docs/user-guide/command-line-interface#--cache

This would probably be done the same as with angular #32 - so the cache is updated after each run.

We should test whether GH Actions can properly cache the shared module. See #15 (comment).

Features:

Must-have (creative alternatives as workarounds are ok):

• zoom in and out (programmatically & mousewheel/pinch to zoom)
• fixed viewport (maximum zoom, no panning outside the viewport), viewport has { latitude, longitude, width, height }
• render elements (= vector graphics and raster graphics) on fixed geographical positions on the map
• eventHandler for click or tap on an element
• reposition an element on the map (click/tap & drag)
• resize elements (click on a corner and drag)
• drag an element on the map (click/tap & drag)
• context-menus (right-click or long-press)
• create edges between transferNodes
• add inputs to these edges (e.g. input field for transferTime, ?viewport? name) - alternative: button that triggers a modal dialog
• high quality satellite world map (able to zoom very far in)

Should-have:

• grouping for svg elements (picture of a vehicle + status indicators + "button")

Nice-to-have:

• calculate the time it takes to travel between two points
• styling support (css like) for all library specific elements
• eventHandler for hovering over elements
• use a png instead of a map (or a custom map)

libraries to investigate (from openstreetmaps)

• Leaflet
• maplibre-gl-js
• openlayers
• Polymaps outdated
• kothic-js outdated

The backend currently supports >=14, all the other packages >=12.

I propose we should require the same node version for all packages (12 is in 3 months EOL anyway).
We could also require a higher node version (16) in general.

Node version overview

See cypress-io/cypress#3090

All visual regression packages for cypress have the same issue.

There is currently an issue when running "All Tests" in Cypress with this plugin. You can follow the progress on the issue here and here. When running "All Tests" any tests that utilize cypress-plugin-snapshots will throw an error.

From cypress-plugin-snapshots - (we are using cypress-image-diff)

As introduced in #47 the backend uses jest tests that currently fail due to our usage of ES modules for the shared library. We therefore had to set a node flag when running backend tests that resulted in the frontend tests failing. We then split up the test running into three separate calls to jest. These should be reunited, at least to allow a global coverage once again, and also better readability of the test output.

Currently, union types in the models are not validated. This should be changed.

See here

To validate our assumptions about the performance of our system we should try to test it in a real scenario as fast as possible. I propose shortly after the end of the first milestone.

• on an experimental branch: add a button that proposes every second a (real) action after one joined a session
• make a button that adds multiple patients instead of just one
• build and host a webserver with our application build from this branch
• an automated client joins the exercise and proposes an action every second
• a real client joins the exercise and move stuff manually around (with optimistic updates enabled) - he can check the latency by seeing opening the website on another tab (that doesn't propose actions but only display the current state on the map).
• figure out the upper limits of the server and clients

Parameters

• number of real clients
• number of automated clients
• number of different devices
• number of different networks (what about VPNs?)
• time between sending an action and receiving the proposal
• subjective smoothness
• any problems?

The differentiation between devDependencies and normal dependencies is actually only relevant for libraries.
Non-library packages (like ours) can make arbitrary rules which go where.
I personally like to differentiate between runtime dependencies (dependencies) and those necessary for building, testing, etc. This convention is also recommended by angular as far as I know.
This makes it easier to spot whether e.g. a DOS vulnerability is affecting your runtime or only a package used for building/testing.

Originally posted by @Dassderdie in #47 (comment)

Angular CLI saves a number of cachable operations on disk. This cache should be reused by our ci.
Our cache is located under frontend/.angular/cache.

Currently every client gets the whole state and it's updates. This could potentially create performance problems on the server/the network.

This is my current stand on it.

We would have to:

1. build the whole application
2. start the frontend and backend
3. run cypress
4. after cypress is finished we have to kill the frontend and backend

See here

By default, socket.io tries HTTP polling and auto reconnecting to a client.
This can result in an action not being transmitted to a client and therefore an invalid state.

• Client options
• Server options

It appears as if the CI uses incorrect commands to build the application.

Exercises should have two ids, one for participants, one for trainer. These ids should be used to determine whether a joining client should be a participant or a trainer. Also, only the trainer id should be deletable by DELETE /api/exercise/:exerciseId.

I followed the steps outlined in README.md to setup the project on a clean system.

When I want to run the Start all task or run some tests I get running tests, the shared folders compiles without problems, but backend and frontend each show a buch of errors of the following kind:

Error: src/app/<SOME_CODE_FILE>.ts:<XX>:<YY> - error TS2307: 
Cannot find module 'digital-fuesim-manv-shared' or its corresponding type declarations.

Trying to delete all local node_modules and installing again did not help.

We should add the client to the state of the exercise upon joining. This includes informing the other clients about the change, i.e. emit a performAction.

The backend has a relatively low test coverage.

A waiting room should be available where all joining participants first get to after joining. Trainer should not go there by default. The waiting room should contain a text informing the participants about the fact where they are.

• Add actionNr to performAction and getState to fix possible synchronisation issues

@Dassderdie
I just noticed a problem with our current API design:
The current idea is to first get the state and then all apply all the actions that we receive via "performAction". But there is no way of knowing which state we get and which actions were already applied.

I would therefore propose the following:

getState and performAction have a timestamp (or some other kind of number that gives them an order) in the answer from the backend.
The frontend

subscribes to performAction and saves the actions it receives
gets the State
Forgets all actions whose timestamp is before the timestamp of the state
Applies all the other actions in order
We would have to check whether a timestamp (Date.now()) is somehow security relevant...

@ClFeSc

I think we could just count the actions and return the action number of the last applied action with the state.

Look here for the original discussion

As dependabot only looks at the main branch its alerts are mostly outdated as we probably already have fixed some of those problems in dev, I'd suggest, additionally to Dependabot, to include auditing in the CI, as an ok-to-fail-job.

See #19 (comment)