initial run state change dialog

[zzstoatzz]

follow up to #17713 and #17743 that adds a generalized component for changing flow or task run states

flow-run-state-dialogue.mov

[codspeed-hq]

CodSpeed Performance Report

Merging #17704 will not alter performance

_{Comparing flow-run-state-dialog (dfc1083) with main (d13684d)}

Summary

✅ 2 untouched benchmarks

[devinvillarosa]

Suggested change

	const { mutateAsync: setFlowRunState } = useSetFlowRunState();
	const { setFlowRunState } = useSetFlowRunState();

suggestion: use the mutate version setFlowRunState

[zzstoatzz]

done! 927ee96

[devinvillarosa]

IMO this return logic makes it complex because this hook will only work if flowRun.state is truthy, (which makes this hook do nothing.).

IMO it be more straightforward if you do something like:

{open && flowRun.state && flowRun.id && <FlowRunDialog id={flowRun.id} state ={flowRun.state} />}

and declare the mutations in the dialog

[zzstoatzz]

disclaimer, just thinking out loud so that if I'm harboring any misunderstanding, it can be corrected!

it seems like this would suggest not having a general run state dialog component, is that right?

I see how using mutate fits if the mutation logic is directly inside the dialog component itself. I had initially separated it out into the reusable RunStateChangeDialog and useFlowRunStateDialog hook after talking w @desertaxle where he suggested a generic RunStateChangeDialog to live in src/components/ui

Since the RunStateChangeDialog awaits the onSubmitChange callback to manage its loading state, I was thinking the hook needed to provide a promise, so i used mutateAsync.

totally possible I'm just too fresh here and I'm only perceiving contention between the suggestions!

[desertaxle]

I think your use of mutateAsync is valid since react-hook-form needs a promise to handle the isSubmitting state, and I think the form being agnostic to the mutation is important for reuse between flow runs and task runs.

I think we can simplify this return statement, but it'll be easier to talk about than to write it out, so let's sync up offline.

[devinvillarosa]

nit: Isn't the the dialog being open derived from the flowRun state being set? Is the open state redundant?

[zzstoatzz]

I did this because onOpenChange (which closes the dialog) just sets open to false without clearing the flowRun state variable inside the hook

[devinvillarosa]

If you want to use that. Then does flowRun need to be a state? Can't that just be passed as a props?

[zzstoatzz]

I kept the flowRun state in the hook to encapsulate all the dialog's context there, in my mind making it simpler for the parent component to just call openDialog but if this

Can't that just be passed as a props

would be preferred, I'm happy to make that change

[devinvillarosa]

Ya, I think that would help simplify things

[zzstoatzz]

cool, should be done in b3ae5d8!

[devinvillarosa]

Suggested change

	interface FlowRunStateDialogProps {
	type FlowRunStateDialogProps {