Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: delete draft detour backend #2925

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

bfauble
Copy link
Contributor

@bfauble bfauble commented Jan 17, 2025

@bfauble bfauble requested a review from a team as a code owner January 17, 2025 22:19
@bfauble bfauble requested review from firestack and removed request for a team January 17, 2025 22:19
@@ -556,6 +557,23 @@ export const fetchDetour = (
ErrStruct: never(),
}).then((v) => map(v, detourStateFromData))

export const deleteDetour = (
id: number | undefined
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: when would id be undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, shouldn't be anymore. the guard for an id already existing is now further up the tree and this typecheck shouldn't be necessary

Comment on lines +185 to +189
const detourPanel: ({
deleteDetourCallback,
}: {
deleteDetourCallback: () => void
}) => React.JSX.Element = () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: What's the benefit of getting this via a parameter instead of pulling it from the scope?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deleteDetourCallback is also making use of the onClose callback being passed into the DiversionPage props, so either way props would be passed into detourPanel but for me it makes sense to have these callbacks/hooks/memos/etc be defined above the rendering of the overall component vs constructing the callback logic at the very last second.

@@ -170,6 +179,13 @@ export const useDraftDetours = (socket: Socket | undefined) => {
})
}

const handleDeleted = (detour_id: number) => {
Copy link
Member

@firestack firestack Jan 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: as much as I love snake_case, we should be using camelCase for variables on the TS side. (if we were more flexible about this, I'd get rid of all our "-Data" structs 😭 )

I'm honestly baffled we don't have a lint for this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes don't quite make sense to me, and part of that is on me for not seeing this in #2909

As is, when the detour.delete.delete-modal.delete-draft event is sent, the state machine will go through the following states

  • "Detour Drawing"."Share Detour"."Deleting"."Confirming" [Start state]
  • "Detour Drawing"."Share Detour"."Deleting"."Deleted"
  • "Detour Drawing"."Share Detour"."Deleting"."Done"
  • "Detour Drawing"."Share Detour"."DoneNoSave"
  • "Detour Drawing"."Active"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh oops this posted before I meant it to.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you might want something like this instead

            Deleting: {
              on: {
                "detour.delete.delete-modal.cancel": {
                  target: "Reviewing",
                },
                "detour.delete.delete-modal.delete-draft": {
                  tags: "no-save",
                  target: "Deleted"
                }
              },
            },
            Deleted: { tags: "no-save" },

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's discuss though!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually meant to ask the best way to approach this "ending" of this workflow. The problem was popping up where the snapshot was attempting to be auto-saved immediately after the delete step. I'll have to work through the steps that actually are fired again, but I think the 'correct' end should be....

Deleted: {
  tags: "no-save",
  type: "final"
}

and remove the onDone and DoneNoSave

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

x-posting/summarizing from slack: it may be better to setup a top level state that halt's the machine and use an id to go to that state from the nested state.

@firestack firestack assigned bfauble and unassigned firestack Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants