Improve routes on map

[patricvincit]

This change is 

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

[Huulivoide]

@Huulivoide reviewed 4 files and all commit messages, and made 7 comments.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on patricvincit).

---

ui/src/components/map/routes/RouteEditor.tsx line 60 at r1 (raw file):

};

const SNAPPING_LINE_LAYER_ID = 'snapping-line';

Tän vois laittaa silleen, että tää on vaan yhdessä paikassa määriteltynä ja sit importataan täällä sisään

---

ui/src/components/map/routes/RouteEditor.tsx line 204 at r1 (raw file):

      if (!creatingNewRoute) {
        onDeleteDrawnRoute();
        removeRoute(map?.getMap(), SNAPPING_LINE_LAYER_ID);

Ylempi onDeleteDrawnRoute kutsuu jo removeRoute:a

---

ui/src/components/map/routes/RouteEditor.tsx line 270 at r1 (raw file):

    if (!creatingNewRoute && drawingMode === Mode.Edit) {
      onDeleteDrawnRoute();
      removeRoute(map?.getMap(), SNAPPING_LINE_LAYER_ID);

onDelete tekee jo tän

---

ui/src/components/map/routes/RouteEditor.tsx line 272 at r1 (raw file):

      removeRoute(map?.getMap(), SNAPPING_LINE_LAYER_ID);
      dispatch(stopRouteEditingAction());
      return;

Tässä vois olla luonnollisempi ihan vaan if-else; return void:in sijaan

---

ui/src/components/map/routes/RouteEditor.tsx line 275 at r1 (raw file):

    }

    onDeleteDrawnRoute();

Tää on nyt eka kutsu seä if lauseen sisällä että sen ulkopuolella. Varmaan nosto funktion ekaks riviks ja sit sopivat if-else osiot

---

ui/src/components/map/routes/RouteEditor.tsx line 278 at r1 (raw file):

    // Remove lingering map layer/source and clear draft geometry after cancel
    removeRoute(map?.getMap(), SNAPPING_LINE_LAYER_ID);

Tääl kansa, ondelete kutsuu jo tätä itsessään

---

ui/src/components/map/routes/RouteEditor.tsx line 279 at r1 (raw file):

    // Remove lingering map layer/source and clear draft geometry after cancel
    removeRoute(map?.getMap(), SNAPPING_LINE_LAYER_ID);
    dispatch(resetDraftRouteGeometryAction());

Tääkin on jo ods onDelete kutsua

[patricvincit]

@patricvincit made 7 comments.
Reviewable status: 0 of 12 files reviewed, 7 unresolved discussions (waiting on Huulivoide).

---

ui/src/components/map/routes/RouteEditor.tsx line 60 at r1 (raw file):

Previously, Huulivoide (Jesse Jaara) wrote…

Tän vois laittaa silleen, että tää on vaan yhdessä paikassa määriteltynä ja sit importataan täällä sisään

Done.

---

ui/src/components/map/routes/RouteEditor.tsx line 204 at r1 (raw file):

Previously, Huulivoide (Jesse Jaara) wrote…

Ylempi onDeleteDrawnRoute kutsuu jo removeRoute:a

Done.

---

ui/src/components/map/routes/RouteEditor.tsx line 270 at r1 (raw file):

Previously, Huulivoide (Jesse Jaara) wrote…

onDelete tekee jo tän

Done.

---

ui/src/components/map/routes/RouteEditor.tsx line 272 at r1 (raw file):

Previously, Huulivoide (Jesse Jaara) wrote…

Tässä vois olla luonnollisempi ihan vaan if-else; return void:in sijaan

Done.

---

ui/src/components/map/routes/RouteEditor.tsx line 275 at r1 (raw file):

Previously, Huulivoide (Jesse Jaara) wrote…

Tää on nyt eka kutsu seä if lauseen sisällä että sen ulkopuolella. Varmaan nosto funktion ekaks riviks ja sit sopivat if-else osiot

Done.

---

ui/src/components/map/routes/RouteEditor.tsx line 278 at r1 (raw file):

Previously, Huulivoide (Jesse Jaara) wrote…

Tääl kansa, ondelete kutsuu jo tätä itsessään

Tässä ei ylempi komponentti enää saanutkaan reffiä, joten uuden reitin luonnissa ondelete ei toimi. Täytyy manuaalisesti poistaa

---

ui/src/components/map/routes/RouteEditor.tsx line 279 at r1 (raw file):

Previously, Huulivoide (Jesse Jaara) wrote…

Tääkin on jo ods onDelete kutsua

tämä samaa kuin tuo ylempi kommentti

[Huulivoide]

@Huulivoide reviewed 10 files, made 1 comment, and resolved 5 discussions.
Reviewable status: 10 of 12 files reviewed, 2 unresolved discussions (waiting on patricvincit).

---

ui/src/components/map/routes/RouteEditor.tsx line 278 at r1 (raw file):

Previously, patricvincit (Patric Kangasmäki) wrote…

Tässä ei ylempi komponentti enää saanutkaan reffiä, joten uuden reitin luonnissa ondelete ei toimi. Täytyy manuaalisesti poistaa

Pikasesti ku luen koodia, niin arvuuttelisin äkkiseltään että uuden reitin piirrossa sen reffin pitäs illa ihan samaan tapaan paikoillaan 🤔. Mutta osaaks sanoo, että onks se ihan lopogisesti oikein että se ei ole siellä paikoillaan uuden piirtossa?

[Huulivoide]

@Huulivoide reviewed 5 files and all commit messages, and made 1 comment.
Reviewable status: 15 of 16 files reviewed, 3 unresolved discussions (waiting on patricvincit).

---

ui/src/components/map/routes/Routes.tsx line 33 at r3 (raw file):

  const renderedRouteIds = selectedRouteId
    ? Array.from(new Set([...displayedRouteIds, selectedRouteId]))

uniq([...displayedRouteIds, selectedRouteId])

[patricvincit]

@patricvincit made 1 comment.
Reviewable status: 13 of 16 files reviewed, 3 unresolved discussions (waiting on Huulivoide).

---

ui/src/components/map/routes/Routes.tsx line 33 at r3 (raw file):

Previously, Huulivoide (Jesse Jaara) wrote…

uniq([...displayedRouteIds, selectedRouteId])

Done.

[patricvincit]

@patricvincit made 1 comment.
Reviewable status: 13 of 16 files reviewed, 3 unresolved discussions (waiting on Huulivoide).

---

ui/src/components/map/routes/RouteEditor.tsx line 278 at r1 (raw file):

Previously, Huulivoide (Jesse Jaara) wrote…

Pikasesti ku luen koodia, niin arvuuttelisin äkkiseltään että uuden reitin piirrossa sen reffin pitäs illa ihan samaan tapaan paikoillaan 🤔. Mutta osaaks sanoo, että onks se ihan lopogisesti oikein että se ei ole siellä paikoillaan uuden piirtossa?

Tuo reffi on riippuvainen DrawRouteLayeristä jossa se alustetaan se useImperativeHandlella. Mutta uutta reittiä luodessa voit painaa peruuta (onCancel) kun ei ole muokkaus käynnissä (eli drawingMode = null), jolloin DrawRouteLayeriä ei renderätä ja sit se reffi ei alustu. Kun se sen ympärillä on tosiaan <Visible visible={drawingMode !== undefined}<

Ja se on kuitenkin OK, että tota peruuta voi painaa ilman, että on se muokkaus käynnissä, niin suoriltaan ei voi tota renderöinnin logiikkaa tai peruuta-painikkeen toiminnallisuuttakaan vaihtaa.

Tässähän toki vois laittaa ton DrawRouteLayerin mounttaamaan uutta reittiä luodessa, mutta sitten ei returnaisi sitä piirroslayeriä vaan tyhjää jos drawingMode on undefined. Olisko sellasessa ratkasussa tolkkua, vai onko enemmän haittaa kuin hyötyä sun mielestä? Siellä kuitenkin sitten ajetaan parit useEffectit

[Huulivoide]

@Huulivoide reviewed 2 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: 15 of 16 files reviewed, 2 unresolved discussions (waiting on patricvincit).

---

ui/src/components/map/routes/RouteEditor.tsx line 278 at r1 (raw file):

Previously, patricvincit (Patric Kangasmäki) wrote…

Tuo reffi on riippuvainen DrawRouteLayeristä jossa se alustetaan se useImperativeHandlella. Mutta uutta reittiä luodessa voit painaa peruuta (onCancel) kun ei ole muokkaus käynnissä (eli drawingMode = null), jolloin DrawRouteLayeriä ei renderätä ja sit se reffi ei alustu. Kun se sen ympärillä on tosiaan <Visible visible={drawingMode !== undefined}<

Ja se on kuitenkin OK, että tota peruuta voi painaa ilman, että on se muokkaus käynnissä, niin suoriltaan ei voi tota renderöinnin logiikkaa tai peruuta-painikkeen toiminnallisuuttakaan vaihtaa.

Tässähän toki vois laittaa ton DrawRouteLayerin mounttaamaan uutta reittiä luodessa, mutta sitten ei returnaisi sitä piirroslayeriä vaan tyhjää jos drawingMode on undefined. Olisko sellasessa ratkasussa tolkkua, vai onko enemmän haittaa kuin hyötyä sun mielestä? Siellä kuitenkin sitten ajetaan parit useEffectit

Kävin myös itse lukemassa tuolta tota koodia ja tosiaan noinhan se siellä on.
Yks muutos tonne pitäs tehdä: DrawRouteLayer komponentin pitäs itse siivota omat tila muutoksensa pois, ku se unmountataan. Toi nyt ikävästi tekee vähän montaa asiaa ja koskee sekä kartaan että redux storeen. Redux storen osalta en osaa ottaa kantaa, koska en tätä koko sekvensointia tässä tunne. Mutta tuolla // Initializing snapping line useEffectissä pitäs palauttaa lopuks cleanup funktio, joka poistaa sen piirto layerin sieltä kartalta eli kutsuu ui/src/utils/map/mapUtils.ts tiedoston removeRoute funktiota siellä SNAPPING_LINE_LAYER_ID id:llä.

Siitä sitten varmaan valuu tänne puolen jotakin muutoksia, että esim tässä nyt ei sit enää tarvi täällä puolen siivota kartalta asioita pois.

[patricvincit]

@patricvincit made 1 comment.
Reviewable status: 11 of 19 files reviewed, 2 unresolved discussions (waiting on Huulivoide).

---

ui/src/components/map/routes/RouteEditor.tsx line 278 at r1 (raw file):

Previously, Huulivoide (Jesse Jaara) wrote…

Kävin myös itse lukemassa tuolta tota koodia ja tosiaan noinhan se siellä on.
Yks muutos tonne pitäs tehdä: DrawRouteLayer komponentin pitäs itse siivota omat tila muutoksensa pois, ku se unmountataan. Toi nyt ikävästi tekee vähän montaa asiaa ja koskee sekä kartaan että redux storeen. Redux storen osalta en osaa ottaa kantaa, koska en tätä koko sekvensointia tässä tunne. Mutta tuolla // Initializing snapping line useEffectissä pitäs palauttaa lopuks cleanup funktio, joka poistaa sen piirto layerin sieltä kartalta eli kutsuu ui/src/utils/map/mapUtils.ts tiedoston removeRoute funktiota siellä SNAPPING_LINE_LAYER_ID id:llä.

Siitä sitten varmaan valuu tänne puolen jotakin muutoksia, että esim tässä nyt ei sit enää tarvi täällä puolen siivota kartalta asioita pois.

Pistän vielä tänne samat asiat mitä tonne kanavalle laitoin tästä. Eli lisäilin

• Cleanupit DrawRouteLayeriin kartan piirolle
• Fallbackina vielä onCanceliin kartan poistaminen, jos se paukahtaa cleanupin jälkeen jostakin vielä näkyviin, esim onko siellä jotain async kutsuja mitä odotellaan ja se tapahtuu cleanupin jälkeen tms.
• Erotellaan editRoutesta tuo editin aloitus ja lopetus omiin funktioihin

[Huulivoide]

@Huulivoide reviewed 6 files, made 1 comment, and resolved 2 discussions.
Reviewable status: 17 of 19 files reviewed, 1 unresolved discussion (waiting on patricvincit).

---

ui/src/components/map/routes/DrawRouteLayer.tsx line 136 at r7 (raw file):

        // to ensure that any pending geometry updates are applied before removing the snapping line from the map
        if (!snappingLine) {
          await debouncedOnAddRoute.flush();

Tänne pitäs pistää isot varotukset että joo, tiedetään että tää on tehty päin perseitä, rikkoo kaikki React koodauksen sääntäjä, hajooa ja paskoo reitin, jos karttaa kluksuttelee liian nopeesti tai jos verkkoyhteys on liian hidas ja todennäköisesti kaataa koko paskan ja jotenkin tulee ja syö lemmikki marsunkin.

[patricvincit]

@patricvincit made 1 comment.
Reviewable status: 17 of 19 files reviewed, 1 unresolved discussion (waiting on Huulivoide).

---

ui/src/components/map/routes/DrawRouteLayer.tsx line 136 at r7 (raw file):

Previously, Huulivoide (Jesse Jaara) wrote…

Tänne pitäs pistää isot varotukset että joo, tiedetään että tää on tehty päin perseitä, rikkoo kaikki React koodauksen sääntäjä, hajooa ja paskoo reitin, jos karttaa kluksuttelee liian nopeesti tai jos verkkoyhteys on liian hidas ja todennäköisesti kaataa koko paskan ja jotenkin tulee ja syö lemmikki marsunkin.

Tän oikeestaan pystyy jättämään kokonaan pois, jos käyttää tuolla onCancelissa tuota useMapin kautta poistoa suoraan. Toki sekin on sellainen, mihin jossakin vaiheessa olisi hyvä puuttua. Jätin siihen TODO kommentin ettei unohdu

[Huulivoide]

@Huulivoide reviewed 2 files and all commit messages, and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on patricvincit).