cleanup(ts/hooks/useDetour): replace `state` with `snapshot` from `createDetourMachine` #2617

firestack · 2024-05-23T12:10:37Z

This used to be #2615 but github thought I closed it >.>

https://github.com/mbta/skate/pull/2607/files#r1608932364

github-actions · 2024-05-23T12:16:20Z

Coverage of commit `2cb4dbe`

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

github-actions · 2024-05-23T12:26:02Z

Coverage of commit `f5c2ec3`

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

github-actions · 2024-05-23T12:26:59Z

Coverage of commit `f5c2ec3`

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

github-actions · 2024-05-23T16:17:13Z

Coverage of commit `54a8fac`

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

github-actions · 2024-05-23T16:17:20Z

Coverage of commit `54a8fac`

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

github-actions · 2024-05-23T16:20:33Z

Coverage of commit `9f5292e`

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

github-actions · 2024-05-23T16:21:01Z

Coverage of commit `9f5292e`

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

github-actions · 2024-05-23T16:24:44Z

Coverage of commit `9f5292e`

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

github-actions · 2024-05-23T16:25:58Z

Coverage of commit `9f5292e`

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

joshlarson · 2024-05-23T18:48:47Z

assets/src/components/detours/diversionPage.tsx

@@ -99,7 +99,7 @@ export const DiversionPage = ({
        </header>

        <div className="l-diversion-page__panel bg-light">
-          {state === DetourState.Edit && (
+          {snapshot.matches({ "Detour Drawing": "Editing" }) ? (


Uprooting a comment thread from the original version of this PR and bringing it here:

Me:

Hmmm - I don't love this, because it leaks the state machine guts out of useDetour and into diversionPage. I like the (imperfect but improving) idea that useDetour's role is to tell diversionPage a bunch of facts about the detour (well, really the detour editing process) as it currently is, and I feel like calling xstate methods directly strays away from that.

I know earlier I said that we should get rid of DetourState, but having something at the level of EditScreen/SharingScreen (and eventually /RouteSelectionScreen) feels like an appropriate thing for useDetour to expose.

@firestack

I somewhat disagree because I see the state machine replacing useDetour/useDetour being a light wrapper around the state machine. I prefer this, because to me the state machines states should ideally match what we're trying to do in the UI, so it should read well to use the state machines states directly in the UI, and IMO, exposing extra states from useDetour that the state machine is unaware of feels wrong in an anti-pattern type way (I could be wrong!), but I think that the state machine is enough of an enum that I'd argue it's typesafe enough to be part of the public api of useDetour and the UI

I think what's throwing me off is that I envisioned the responsibilities of the components as something like:

state machine does all the state management, like holding onto data and figuring out how to handle state transitions. Ya know - state machine stuff.

useDetour maps that onto ... sort of a description of what's on the screen. Things like "the start point is here", and "the undo button should do this thing" (or "the undo button should be disabled right now")

<DiversionPage /> and its friends <DetourMap /> and the various <*Panel />s handle the layout, but don't do any interpretation beyond putting the things from useDetour in the right place on the screen.

So to me, the mapping of "this is the xstate state we're in" to "throw the whatever panel on the left" maps cleanly onto useDetour's responsibilities. This code makes <DiversionPage /> do that instead, which feels like it cuts through the boundaries ☝️ differently from everything else.

The original reason I had this thought was because I envisioned useDetour as a (thin) wrapper around the state machine, and this particular thing pokes through that wrapper. Everything ☝️ ☝️ ☝️ is, I think, why I kind of want to stand by my instinct here.

I would also maybe be open to totally removing useDetour and putting <DiversionPage /> in charge of the "xstate state" <-> "UI description" mapping instead.

I'm not totally sold on that, because I think that there's value in keeping the layout details and the page description separate from each other (and I don't want <DiversionPage /> to get too bloated), but there might also be a bit of a fear-of-change thing going on, so I'd be interested to see how it looks if we decided to try it.

I would also maybe be open to totally removing useDetour and putting in charge of the "xstate state" <-> "UI description" mapping instead.

...what if we just get rid of useDetour? 😅 I'm in favor of this :) especially as I'm having to do a lot of the same calls in useDetour as the diversion page now.

I'll have something for us to chat about soon which I think might help here!

Can't wait!

https://mbta.slack.com/archives/C047ZT8ACP5/p1716994945665709?thread_ts=1716992038.426239&cid=C047ZT8ACP5

@joshlarson, given our conversation, do you think there's any more to do here on this PR?

Oh woops! Lemme give this another 👀

github-actions · 2024-05-23T21:59:16Z

Coverage of commit `4c060cb`

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

…eateDetourMachine`

github-actions · 2024-05-29T17:15:51Z

Coverage of commit `27b4632`

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

firestack self-assigned this May 23, 2024

firestack requested a review from a team as a code owner May 23, 2024 12:10

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

firestack force-pushed the kf/asn/state-machine/state-replacement branch from 2cb4dbe to f5c2ec3 Compare May 23, 2024 12:20

firestack mentioned this pull request May 23, 2024

cleanup(ts/hooks/useDetour): replace state with snapshot from createDetourMachine #2615

Merged

firestack force-pushed the kf/asn/state-machine/state-var branch from ed07886 to 5f13e11 Compare May 23, 2024 16:11

firestack force-pushed the kf/asn/state-machine/state-replacement branch from f5c2ec3 to 2b8a7f1 Compare May 23, 2024 16:11

firestack force-pushed the kf/asn/state-machine/state-var branch from 5f13e11 to d933c71 Compare May 23, 2024 16:14

firestack force-pushed the kf/asn/state-machine/state-replacement branch from 2b8a7f1 to 54a8fac Compare May 23, 2024 16:14

firestack force-pushed the kf/asn/state-machine/state-var branch from d933c71 to cfdc141 Compare May 23, 2024 16:19

firestack force-pushed the kf/asn/state-machine/state-replacement branch from 54a8fac to 9f5292e Compare May 23, 2024 16:19

Base automatically changed from kf/asn/state-machine/state-var to main May 23, 2024 16:24

joshlarson reviewed May 23, 2024

View reviewed changes

firestack force-pushed the kf/asn/state-machine/state-replacement branch from 9f5292e to 4c060cb Compare May 23, 2024 21:52

cleanup(ts/hooks/useDetour): replace state with snapshot from `cr…

27b4632

…eateDetourMachine`

firestack force-pushed the kf/asn/state-machine/state-replacement branch from 4c060cb to 27b4632 Compare May 29, 2024 17:10

joshlarson approved these changes May 29, 2024

View reviewed changes

firestack merged commit 2d565c8 into main May 29, 2024
21 checks passed

firestack deleted the kf/asn/state-machine/state-replacement branch May 29, 2024 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cleanup(ts/hooks/useDetour): replace `state` with `snapshot` from `createDetourMachine` #2617

cleanup(ts/hooks/useDetour): replace `state` with `snapshot` from `createDetourMachine` #2617

firestack commented May 23, 2024 •

edited

Loading

github-actions bot commented May 23, 2024

github-actions bot commented May 23, 2024

github-actions bot commented May 23, 2024

github-actions bot commented May 23, 2024

github-actions bot commented May 23, 2024

github-actions bot commented May 23, 2024

github-actions bot commented May 23, 2024

github-actions bot commented May 23, 2024

github-actions bot commented May 23, 2024

joshlarson May 23, 2024

joshlarson May 23, 2024

firestack May 23, 2024 •

edited

Loading

firestack May 23, 2024

joshlarson May 23, 2024

firestack May 29, 2024

firestack May 29, 2024 •

edited

Loading

joshlarson May 29, 2024

github-actions bot commented May 23, 2024

github-actions bot commented May 29, 2024

cleanup(ts/hooks/useDetour): replace state with snapshot from createDetourMachine #2617

cleanup(ts/hooks/useDetour): replace state with snapshot from createDetourMachine #2617

Conversation

firestack commented May 23, 2024 • edited Loading

github-actions bot commented May 23, 2024

Coverage of commit 2cb4dbe

github-actions bot commented May 23, 2024

Coverage of commit f5c2ec3

github-actions bot commented May 23, 2024

Coverage of commit f5c2ec3

github-actions bot commented May 23, 2024

Coverage of commit 54a8fac

github-actions bot commented May 23, 2024

Coverage of commit 54a8fac

github-actions bot commented May 23, 2024

Coverage of commit 9f5292e

github-actions bot commented May 23, 2024

Coverage of commit 9f5292e

github-actions bot commented May 23, 2024

Coverage of commit 9f5292e

github-actions bot commented May 23, 2024

Coverage of commit 9f5292e

joshlarson May 23, 2024

Choose a reason for hiding this comment

joshlarson May 23, 2024

Choose a reason for hiding this comment

firestack May 23, 2024 • edited Loading

Choose a reason for hiding this comment

firestack May 23, 2024

Choose a reason for hiding this comment

joshlarson May 23, 2024

Choose a reason for hiding this comment

firestack May 29, 2024

Choose a reason for hiding this comment

firestack May 29, 2024 • edited Loading

Choose a reason for hiding this comment

joshlarson May 29, 2024

Choose a reason for hiding this comment

github-actions bot commented May 23, 2024

Coverage of commit 4c060cb

github-actions bot commented May 29, 2024

Coverage of commit 27b4632

cleanup(ts/hooks/useDetour): replace `state` with `snapshot` from `createDetourMachine` #2617

cleanup(ts/hooks/useDetour): replace `state` with `snapshot` from `createDetourMachine` #2617

firestack commented May 23, 2024 •

edited

Loading

Coverage of commit `2cb4dbe`

Coverage of commit `f5c2ec3`

Coverage of commit `f5c2ec3`

Coverage of commit `54a8fac`

Coverage of commit `54a8fac`

Coverage of commit `9f5292e`

Coverage of commit `9f5292e`

Coverage of commit `9f5292e`

Coverage of commit `9f5292e`

firestack May 23, 2024 •

edited

Loading

firestack May 29, 2024 •

edited

Loading

Coverage of commit `4c060cb`

Coverage of commit `27b4632`