feat(ts/hooks/useDetour): control waypoints with state machine #2607

firestack · 2024-05-20T12:35:53Z

Summary

Part 2 of the conversion of useDetour to a state machine!
This converts the waypoint (including startPoint and endPoint) to be managed by a state machine, instead of manual logic for managing the state. (Considering revisiting this decision about start and end points!)

Instead of managing separate startPoint and endPoints, they're now derived from the waypoints array, this is done so that startPoint and endPoint are no longer containing implicit state of the status of the machine.

Review Notes

This PR involves a few dependencies and steps.

Depends on:

cleanup:feat(ts/hooks/useDetour): convert fetchFinishedDetour to use useApiCall #2605
- Makes the diff on this PR a bit easier to read
fix:test(ts/components/diversionPage): add waitFor for expect #2602
- Fixes a test that would otherwise just be noise in this PR
feat(ts/hooks/useDetour): replace state variable with state machine #2616
- First introduces XState and an initial conversion

This PR was done in these steps and such could be reviewed in that order to make the diff a little easier to read.

I optionally have cleanup(ts/hooks/useDetour): convert complex match to tags this to simplify the match logic for finishedDetour

~~If desired, feat(ts/hooks/useDetour): replace state variable with state machine and feat(ts/hooks/useDetour): control waypoints with state machine can be split into two PRs~~

Edit:
This has been broken into another PR

feat(ts/hooks/useDetour): replace state variable with state machine #2616

Asana Ticket: https://app.asana.com/0/0/1207381767522634/f

github-actions · 2024-05-20T12:41:08Z

Coverage of commit `67f6d95`

Summary coverage rate:
  lines......: 93.5% (3240 of 3464 lines)
  functions..: 73.3% (1339 of 1827 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

assets/src/hooks/useDetour.ts

github-actions · 2024-05-20T13:37:33Z

Coverage of commit `cd015ef`

Summary coverage rate:
  lines......: 93.6% (3241 of 3464 lines)
  functions..: 73.3% (1340 of 1827 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

github-actions · 2024-05-20T13:37:40Z

Coverage of commit `cd015ef`

Summary coverage rate:
  lines......: 93.5% (3240 of 3464 lines)
  functions..: 73.3% (1339 of 1827 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

assets/src/hooks/useDetour.ts

github-actions · 2024-05-21T21:58:45Z

Coverage of commit `c9f4c62`

Summary coverage rate:
  lines......: 93.5% (3241 of 3465 lines)
  functions..: 73.3% (1340 of 1828 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

joshlarson · 2024-05-21T23:58:52Z

assets/src/hooks/useDetour.ts

+       * > strongly-typed machines can still be achieved without Typegen.
+       * > -- https://stately.ai/docs/migration#use-typestypegen-instead-of-tstypes
+       */
+      if (


Near the bottom, we're still computing startPoint and endPoint (using a slightly different formula that I think will always give the same answer, assuming the state machine states are set up correctly).

Why not just compute startPoint and endPoint as values computed from snapshot? So something like:

const startPoint = snapshot.context.waypoints.at(0) const endPoint = snapshot.matches({ "Detour Drawing": { Editing: "Finished Drawing" }, }) && snapshot.context.waypoints.at(-1) /* ... */ if (startPoint && endPoint) { return fetchFinishedDetour(routePatternId, startPoint, endPoint) } /* ... */ return { startPoint, endPoint: endPoint || undefined, /* ... */ }

I tried to do this in 9c4e8a9 (#2607)
Let me know what you think!

assets/src/hooks/useDetour.ts

assets/src/models/createDetourMachine.ts

joshlarson · 2024-05-22T00:20:51Z

This looks so good! I'm so excited to see how this makes the rest of our work on detours easier!

If desired, feat(ts/hooks/useDetour): replace state variable with state machine and feat(ts/hooks/useDetour): control waypoints with state machine can be split into two PRs.

I think that splitting this in that way would be a good idea. I have a few reasons for thinking this:

This whole thing was a lot easier to review commit-by-commit (well, those two commits - the other commits are mostly fixes and prework that would disappear in a rebase), which means that those commits make sense as standalone work.
I'm pretty sure the first PR by itself unblocks the detour route selection panel.
I think the first PR is either ready to merge or very very close, while there are a bunch of comments on the second one that I'd be interested in resolving before merging it.

github-actions · 2024-05-22T00:41:20Z

Coverage of commit `0ba679d`

Summary coverage rate:
  lines......: 93.6% (3242 of 3465 lines)
  functions..: 73.4% (1341 of 1828 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

firestack · 2024-05-22T12:20:50Z

If desired, feat(ts/hooks/useDetour): replace state variable with state machine and feat(ts/hooks/useDetour): control waypoints with state machine can be split into two PRs.

I think that splitting this in that way would be a good idea. I have a few reasons for thinking this:

I'm pretty sure the first PR by itself unblocks the detour route selection panel.

I've opened up a PR for the first half #2616

assets/src/models/createDetourMachine.ts

assets/src/hooks/useDetour.ts

assets/src/models/createDetourMachine.ts

joshlarson · 2024-05-23T22:43:38Z

assets/src/models/createDetourMachine.ts

+    events:
+      | { type: "detour.edit.clear-detour" }
+      | { type: "detour.edit.done" }
+      | { type: "detour.edit.place-connection-point"; location: ShapePoint }


We've discussed before that "Connection Point" actually means something different (the last un-missed stop before the detour begins and the first un-missed stop after the detour ends). Should we take this opportunity to come up with a different name?

Suggested change

| { type: "detour.edit.place-connection-point"; location: ShapePoint }

| { type: "detour.edit.place-point-on-original-route"; location: ShapePoint }

☝️ a possibility, although honestly... we can do better 😅

how about detour.edit.place-detour-terminus-point?

I'm mainly thinking that we may have "ghost" connection points, that are on the original route, so I'm trying to get at the root user intention.

Though terminus is not great :/ looking for something to generalize the idea of "start" and "end"

👻 I'm not sure I understand "ghost" connection points on the original route? What is that? 👻

Basically the ideas behind https://app.asana.com/0/1148853526253420/1207383192300456/f

We'd show the user connection points where they placed them, but the "ghost" connection points are the ones we actually use.

Oh, huh.

Maybe I'm missing something, but isn't that a different question? Like, we're displaying the points in one spot, and computing them effectively from a different spot, but either way, the question is... what should they be called?

github-actions · 2024-05-29T17:29:48Z

Coverage of commit `b684c6d`

Summary coverage rate:
  lines......: 93.6% (3242 of 3465 lines)
  functions..: 73.4% (1341 of 1828 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

hannahpurcell

This has been greatly simplified since I last reviewed! I found it easier to read 👍

joshlarson

Looks great!

It might be worth figuring out a better name per this comment --> #2607 (comment) <--, although I wouldn't want to block on that, since I don't have any good ideas 🙃

It might also be worth waiting until #2645 is merged, because I think they'll conflict, and #2645 blocks our current sprint goal.

Either way - those are both up to you - this PR looks excellent as it is!

new commits

github-actions · 2024-06-10T15:41:02Z

Coverage of commit `340f277`

Summary coverage rate:
  lines......: 93.6% (3257 of 3480 lines)
  functions..: 73.4% (1343 of 1830 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

github-actions · 2024-06-10T15:46:15Z

Coverage of commit `340f277`

Summary coverage rate:
  lines......: 93.6% (3257 of 3480 lines)
  functions..: 73.4% (1343 of 1830 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

Co-authored-by: Josh Larson <jlarson@mbta.com>

github-actions · 2024-06-10T20:28:42Z

Coverage of commit `59c8e6b`

Summary coverage rate:
  lines......: 93.6% (3257 of 3480 lines)
  functions..: 73.4% (1343 of 1830 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

firestack self-assigned this May 20, 2024

firestack marked this pull request as ready for review May 20, 2024 12:52

firestack requested a review from a team as a code owner May 20, 2024 12:52

firestack commented May 20, 2024

View reviewed changes

assets/src/hooks/useDetour.ts Show resolved Hide resolved

firestack force-pushed the kf/asn/state-machine/waypoints branch from 67f6d95 to cd015ef Compare May 20, 2024 13:30

firestack force-pushed the kf/cleanup/convert-finished-useapi branch from 6ab1fed to 8378733 Compare May 20, 2024 13:30

Base automatically changed from kf/cleanup/convert-finished-useapi to main May 20, 2024 17:53

firestack commented May 21, 2024

View reviewed changes

assets/src/hooks/useDetour.ts Outdated Show resolved Hide resolved

joshlarson reviewed May 21, 2024

View reviewed changes

assets/src/hooks/useDetour.ts Outdated Show resolved Hide resolved

firestack force-pushed the kf/asn/state-machine/waypoints branch from cd015ef to c9f4c62 Compare May 21, 2024 21:51

joshlarson reviewed May 21, 2024

View reviewed changes

joshlarson reviewed May 22, 2024

View reviewed changes

assets/src/hooks/useDetour.ts Outdated Show resolved Hide resolved

joshlarson reviewed May 22, 2024

View reviewed changes

assets/src/hooks/useDetour.ts Outdated Show resolved Hide resolved

joshlarson reviewed May 22, 2024

View reviewed changes

assets/src/models/createDetourMachine.ts Outdated Show resolved Hide resolved

firestack mentioned this pull request May 22, 2024

feat(ts/hooks/useDetour): replace state variable with state machine #2616

Merged

firestack changed the base branch from main to kf/asn/state-machine/state-var May 22, 2024 00:29

firestack force-pushed the kf/asn/state-machine/waypoints branch 2 times, most recently from 9dd609f to 0ba679d Compare May 22, 2024 00:35

firestack changed the title ~~cleanup:feat(ts/hooks/useDetour): convert useDetour to use State Chart~~ feat(ts/hooks/useDetour): control waypoints with state machine May 22, 2024

hannahpurcell reviewed May 22, 2024

View reviewed changes

assets/src/models/createDetourMachine.ts Show resolved Hide resolved

firestack force-pushed the kf/asn/state-machine/waypoints branch from 0ba679d to 588dcc8 Compare May 23, 2024 12:01

firestack force-pushed the kf/asn/state-machine/state-var branch from e8b1719 to c5999e0 Compare May 23, 2024 12:01

firestack mentioned this pull request May 23, 2024

cleanup(ts/hooks/useDetour): convert compound match to tags #2606

Closed

joshlarson reviewed May 23, 2024

View reviewed changes

assets/src/hooks/useDetour.ts Outdated Show resolved Hide resolved

joshlarson reviewed May 23, 2024

View reviewed changes

assets/src/hooks/useDetour.ts Outdated Show resolved Hide resolved

joshlarson reviewed May 23, 2024

View reviewed changes

assets/src/hooks/useDetour.ts Show resolved Hide resolved

joshlarson reviewed May 23, 2024

View reviewed changes

assets/src/models/createDetourMachine.ts Outdated Show resolved Hide resolved

joshlarson reviewed May 23, 2024

View reviewed changes

firestack force-pushed the kf/asn/state-machine/waypoints branch from b82164e to b684c6d Compare May 29, 2024 17:24

hannahpurcell previously approved these changes Jun 3, 2024

View reviewed changes

firestack mentioned this pull request Jun 5, 2024

refactor: Store startPoint, waypoints, and endPoint as separate pieces of context #2629

Closed

firestack requested a review from joshlarson June 5, 2024 14:05

joshlarson previously approved these changes Jun 5, 2024

View reviewed changes

firestack force-pushed the kf/asn/state-machine/waypoints branch 2 times, most recently from 829ccc3 to 340f277 Compare June 10, 2024 15:39

firestack assigned joshlarson and hannahpurcell and unassigned firestack Jun 10, 2024

firestack requested review from joshlarson and hannahpurcell June 10, 2024 15:40

firestack mentioned this pull request Jun 10, 2024

fix(ts/models/createDetourMachine): clear detour when selecting route pattern #2658

Merged

joshlarson approved these changes Jun 10, 2024

View reviewed changes

feat(ts/hooks/useDetour): control waypoints with state machine

59c8e6b

Co-authored-by: Josh Larson <jlarson@mbta.com>

firestack force-pushed the kf/asn/state-machine/waypoints branch from 340f277 to 59c8e6b Compare June 10, 2024 20:22

hannahpurcell approved these changes Jun 11, 2024

View reviewed changes

firestack merged commit ddd6728 into main Jun 11, 2024
15 checks passed

firestack deleted the kf/asn/state-machine/waypoints branch June 11, 2024 18:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ts/hooks/useDetour): control waypoints with state machine #2607

feat(ts/hooks/useDetour): control waypoints with state machine #2607

firestack commented May 20, 2024 •

edited

Loading

github-actions bot commented May 20, 2024

github-actions bot commented May 20, 2024

github-actions bot commented May 20, 2024

github-actions bot commented May 21, 2024

joshlarson May 21, 2024

firestack May 23, 2024

joshlarson commented May 22, 2024

github-actions bot commented May 22, 2024

firestack commented May 22, 2024

joshlarson May 23, 2024

firestack Jun 3, 2024

firestack Jun 3, 2024

firestack Jun 3, 2024

joshlarson Jun 5, 2024

firestack Jun 5, 2024

joshlarson Jun 5, 2024

github-actions bot commented May 29, 2024

hannahpurcell left a comment

joshlarson left a comment

github-actions bot commented Jun 10, 2024

github-actions bot commented Jun 10, 2024

github-actions bot commented Jun 10, 2024

	\| { type: "detour.edit.place-connection-point"; location: ShapePoint }
	\| { type: "detour.edit.place-point-on-original-route"; location: ShapePoint }

feat(ts/hooks/useDetour): control waypoints with state machine #2607

feat(ts/hooks/useDetour): control waypoints with state machine #2607

Conversation

firestack commented May 20, 2024 • edited Loading

Summary

Review Notes

github-actions bot commented May 20, 2024

Coverage of commit 67f6d95

github-actions bot commented May 20, 2024

Coverage of commit cd015ef

github-actions bot commented May 20, 2024

Coverage of commit cd015ef

github-actions bot commented May 21, 2024

Coverage of commit c9f4c62

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshlarson commented May 22, 2024

github-actions bot commented May 22, 2024

Coverage of commit 0ba679d

firestack commented May 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 29, 2024

Coverage of commit b684c6d

hannahpurcell left a comment

Choose a reason for hiding this comment

joshlarson left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 10, 2024

Coverage of commit 340f277

github-actions bot commented Jun 10, 2024

Coverage of commit 340f277

github-actions bot commented Jun 10, 2024

Coverage of commit 59c8e6b

firestack commented May 20, 2024 •

edited

Loading

Coverage of commit `67f6d95`

Coverage of commit `cd015ef`

Coverage of commit `cd015ef`

Coverage of commit `c9f4c62`

Coverage of commit `0ba679d`

Coverage of commit `b684c6d`

Coverage of commit `340f277`

Coverage of commit `340f277`

Coverage of commit `59c8e6b`