[24.1] Handle error when workflow is unowned in Invocation view

[ahmedhamidawan]

Found this when investigating #18697; that if (in this case) I run another user's shared workflow with a subworkflow, the invocation view for the parent renders everything fine, but there is an unhandled error caused by the subworkflow not being shared. Now this fix handles that error and shows it on hover on the WorkflowStepTitle:

invocation_view_unowned_workflow.mp4

How to test the changes?

(Select all options that apply)

• I've included appropriate automated tests.
• This is a refactoring of components with existing test coverage.
• Instructions for manual testing are as follows:
  1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

• I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

[mvdbeek]

More error handling is always great, thanks @ahmedhamidawan! We should also allow access to embedded workflows if we have access to the parent workflow.

[ahmedhamidawan]

In commit a24c4e3 I added a Messages tab to the WorkflowInvocationState view as well, in case we have a lot of messages from an invocation. Otherwise, this would make for a bad UI with the messages taking up space at the top above the InvocationGraph

Here is a Before (left) and After (right):

invocation_view_add_messages_tab.mp4

If this is too much to target 24.1, I can remove this commit from here and put this in a separate PR for dev?

[mvdbeek]

I think this must be at the top, this is the most importantly message when it is present and there should only be one of we're not running into backend bugs.

[ahmedhamidawan]

I think this must be at the top, this is the most importantly message when it is present...

Oh it still remains at the top but only the first one, the rest are put into a tab that is highlighted as shown

...and there should only be one of we're not running into backend bugs.

Ah, in that case this might be redundant then, if we're expecting a max of 1. If there is only 1 message the separate tab is never rendered and nothing changes. The change is only for more than 1 message.

So, what should we do, should i just drop this commit?

[mvdbeek]

I would prefer that, and maybe do something like const uniqueMessages = Array.from(new Set(messages)) for existing invocations ?

[mvdbeek]

If all function calls in an async function are awaited, is it even async ?

Also in this case useWorkflowInstance would be much preferable. If you want to improve the error handling there you could easily return a third object error in https://github.com/galaxyproject/galaxy/blob/dev/client/src/composables/useWorkflowInstance.ts#L16

[ElectronicBlueberry]

If all function calls in an async function are awaited, is it even async ?

Yes it is. The rest of the event loop continues to execute while the execution of this specific function is waiting.

In fact, the opposite is true:

If no function call in an async function is awaited, it is not really async.

[mvdbeek]

Yes it is. The rest of the event loop continues to execute while the execution of this specific function is waiting.

Correct me if I'm wrong, but the calls here will be executed linearly, just like in a sync function (yes, event loop continues) ? None of these calls however need to wait on each other, they're completely independent.

[ElectronicBlueberry]

Correct me if I'm wrong, but the calls here will be executed linearly, just like in a sync function

Which is the purpose of async functions. If no await is used, there is no need for the calling function to be async.

None of these calls however need to wait on each other

the next line depends on this line.

[ahmedhamidawan]

the next line depends on this line

... is what I understood by using await calls as well, hence, I used them here.

[mvdbeek]

So we're all on the same page that, if useWorkflowInstance is used, there's no need for this to be an async function, because nothing needs to be awaited ?

[ahmedhamidawan]

The reason I didn't just place useWorkflowInstance outside of the onMounted hook and the if statement is because when this component is initialized, we have no invocation, and hence, no invocation.workflow_id, and so useWorkflowInstance(invocation.value?.workflow_id) causes an error (no workflow id).
Therefore, I added an await fetchInvocationForId and once that gives us the invocation, we fetch the workflow

I could replace that individual line:

Suggested change

	await workflowStore.fetchWorkflowForInstanceId(invocation.value.workflow_id);
	const { workflow: workflowIns } = useWorkflowInstance(invocation.value.workflow_id);
	workflow.value = workflowIns.value;

[ahmedhamidawan]

I could also potentially set workflow as a computed ref like so:

const workflow = computed(() => {
    if (invocation.value) {
        const { workflow } = useWorkflowInstance(invocation.value.workflow_id);
        return workflow.value;
    }
    return undefined;
});
const isDeletedWorkflow = computed(() => workflow.value?.deleted === true);
const workflowVersion = computed(() => workflow.value?.version);

onMounted(async () => {
    try {
        await invocationStore.fetchInvocationForId({ id: props.invocationId });
        invocationLoaded.value = true;
        if (invocation.value) {
            await pollStepStatesUntilTerminal();
            await pollJobStatesUntilTerminal();
        }
    } catch (e) {
        errorMessage.value = errorMessageAsString(e);
    } finally {
        initialLoading.value = false;
    }
});

but the reason I had it the way it was in the singular mounted hook for fetching invocation AND workflow, was because we were catching errors from either there. That's why I changed invocation from a computed getInvocationForId to what we have now as well

[mvdbeek]

I think it'd be a significant improvement to move this all out of the onMounted hook. The computed looks good to me (just need to return error in useWorkflowInstance).

[ahmedhamidawan]

We are now not computing/getting the workflow in the parent WorkflowInvocationState component at all
Instead, we use it in a child WorkflowInvocationHeader and the existing WorkflowInvocationOverview where we use useWorkflowInstance which expects an invocation.workflow_id which we are ensuring exists in the parent.

[mvdbeek]

Awesome, thanks a lot for making all those changes!