implement extensive typing for workflow refactoring methods

Adds an entire `lintingTypes.d.ts` file to store not just the schema defined refactor request types for each action, but also all types of "lint state" objects generated on the client for those requests.

Author: ahmedhamidawan

client/src/api/workflows.ts

@@ -4,6 +4,9 @@ import { rethrowSimple } from "@/utils/simple-error";
 import { GalaxyApi } from "./client";
 
 export type Creator = components["schemas"]["Person"] | components["schemas"]["CreatorOrganization"];
+export type RefactorRequestAction = components["schemas"]["RefactorRequest"]["actions"][number];
+export type RefactorResponse = components["schemas"]["RefactorResponse"];
+export type RefactorResponseActionExecution = RefactorResponse["action_executions"][number];
 export type StoredWorkflowDetailed = components["schemas"]["StoredWorkflowDetailed"];
 export type WorkflowStepTyped = StoredWorkflowDetailed["steps"][number];
 
@@ -107,6 +110,24 @@ export async function getWorkflowInfo(workflowId: string, version?: number, inst
     return data;
 }
 
+export async function refactor(id: string, actions: RefactorRequestAction[], style: string, dryRun = false) {
+    const { data, error } = await GalaxyApi().PUT("/api/workflows/{workflow_id}/refactor", {
+        params: {
+            path: { workflow_id: id },
+        },
+        body: {
+            actions: actions,
+            style: style,
+            dry_run: dryRun,
+        },
+    });
+    if (error) {
+        rethrowSimple(error);
+    }
+
+    return data as RefactorResponse;
+}
+
 export async function undeleteWorkflow(id: string): Promise<WorkflowSummary> {
     const { data, error } = await GalaxyApi().POST("/api/workflows/{workflow_id}/undelete", {
         params: {

client/src/components/Workflow/Editor/Lint.vue

@@ -8,7 +8,6 @@ import { useWorkflowStores } from "@/composables/workflowStores";
 import type { Steps } from "@/stores/workflowStepStore";
 
 import type { Rectangle } from "./modules/geometry";
-import type { LintState } from "./modules/linting";
 import {
     bestPracticeWarningAnnotation,
     bestPracticeWarningAnnotationLength,
@@ -19,7 +18,9 @@ import {
     fixDisconnectedInput,
     fixUnlabeledOutputs,
     fixUntypedParameter,
+    isStateForInputOrOutput,
 } from "./modules/linting";
+import type { DisconnectedInputState, LintState, UntypedParameterState } from "./modules/lintingTypes";
 import type { LintData } from "./modules/useLinting";
 
 import LintSectionSeparator from "./LintSectionSeparator.vue";
@@ -123,7 +124,7 @@ async function onFixUntypedParameter(item: LintState) {
     );
 
     if (confirmed) {
-        emit("onRefactor", [fixUntypedParameter(item)]);
+        emit("onRefactor", [fixUntypedParameter(item as UntypedParameterState)]);
     } else {
         emit("onScrollTo", item.stepId);
     }
@@ -145,7 +146,7 @@ async function onFixDisconnectedInput(item: LintState) {
         "Fix Disconnected Input",
     );
     if (confirmed) {
-        emit("onRefactor", [fixDisconnectedInput(item)]);
+        emit("onRefactor", [fixDisconnectedInput(item as DisconnectedInputState)]);
     } else {
         emit("onScrollTo", item.stepId);
     }
@@ -169,7 +170,11 @@ function openAndFocus(item: LintState) {
 }
 
 function onHighlight(item: LintState) {
-    const bounds = searchStore.getBoundsForItemCached(item.stepId, item.highlightType || "step", item.name);
+    const bounds = searchStore.getBoundsForItemCached(
+        item.stepId,
+        isStateForInputOrOutput(item) ? item.highlightType : "step",
+        "name" in item ? item.name : undefined,
+    );
     if (bounds) {
         emit("onHighlightRegion", bounds);
     }

client/src/components/Workflow/Editor/LintSection.vue

@@ -10,7 +10,8 @@ import {
 import { FontAwesomeIcon } from "@fortawesome/vue-fontawesome";
 import { computed } from "vue";
 
-import { dataAttributes, type LintState } from "./modules/linting";
+import { dataAttributes } from "./modules/linting";
+import type { LintState } from "./modules/lintingTypes";
 
 import GButton from "@/components/BaseComponents/GButton.vue";
 import GLink from "@/components/BaseComponents/GLink.vue";
@@ -77,7 +78,7 @@ const isOkay = computed(() => props.okay && !hasWarningItems.value);
                         data-description="autofix item link"
                         v-bind="dataAttributes(item)"
                         @click="emit('onClick', item)">
-                        <FontAwesomeIcon v-if="item.autofix" :icon="faMagic" class="mr-1" />
+                        <FontAwesomeIcon v-if="'autofix' in item && item.autofix" :icon="faMagic" class="mr-1" />
                         <FontAwesomeIcon v-else :icon="faSearch" class="mr-1" />
                         <strong>Step {{ item.stepId + 1 }}</strong>
                         {{ item.stepLabel }}: {{ item.warningLabel }}

client/src/components/Workflow/Editor/RefactorConfirmationModal.test.ts

@@ -3,12 +3,12 @@ import { shallowMount, type Wrapper } from "@vue/test-utils";
 import flushPromises from "flush-promises";
 import { beforeEach, describe, expect, it, vi } from "vitest";
 
-import { refactor } from "./modules/services";
+import { refactor, type RefactorResponse } from "@/api/workflows";
 
 import RefactorConfirmationModal from "./RefactorConfirmationModal.vue";
 import GModal from "@/components/BaseComponents/GModal.vue";
 
-vi.mock("./modules/services", () => ({
+vi.mock("@/api/workflows", () => ({
     refactor: vi.fn(),
 }));
 
@@ -49,7 +49,7 @@ describe("RefactorConfirmationModal.vue", () => {
         await flushPromises();
         expect(wrapper.emitted().onWorkflowError!.length).toBe(1);
         expect(vi.mocked(refactor).mock.calls[0]![0]).toEqual(TEST_WORKFLOW_ID);
-        expect(vi.mocked(refactor).mock.calls[0]![2]).toBeTruthy(); // dry run argument
+        expect(vi.mocked(refactor).mock.calls[0]![3]).toBeTruthy(); // dry run argument
 
         // onRefactor never emitted because there was a failure
         expect(wrapper.emitted().onRefactor).toBeFalsy();
@@ -60,7 +60,7 @@ describe("RefactorConfirmationModal.vue", () => {
             new Promise((then) => {
                 then({
                     action_executions: [],
-                });
+                } as unknown as RefactorResponse);
             }),
         );
         await wrapper.setProps({
@@ -70,10 +70,10 @@ describe("RefactorConfirmationModal.vue", () => {
         expect(wrapper.emitted().onWorkflowError).toBeFalsy();
         // called with dry run as true...
         expect(vi.mocked(refactor).mock.calls[0]![0]).toEqual(TEST_WORKFLOW_ID);
-        expect(vi.mocked(refactor).mock.calls[0]![2]).toBeTruthy();
+        expect(vi.mocked(refactor).mock.calls[0]![3]).toBeTruthy();
         // ... and then as false
         expect(vi.mocked(refactor).mock.calls[1]![0]).toEqual(TEST_WORKFLOW_ID);
-        expect(vi.mocked(refactor).mock.calls[1]![2]).toBeFalsy();
+        expect(vi.mocked(refactor).mock.calls[1]![3]).toBeFalsy();
         // second time onRefactor emitted with the final response
         expect(wrapper.emitted().onRefactor!.length).toBe(1);
     });
@@ -93,7 +93,7 @@ describe("RefactorConfirmationModal.vue", () => {
                             ],
                         },
                     ],
-                });
+                } as unknown as RefactorResponse);
             }),
         );
 
@@ -108,7 +108,7 @@ describe("RefactorConfirmationModal.vue", () => {
 
         // called with dry run...
         expect(vi.mocked(refactor).mock.calls[0]![0]).toEqual(TEST_WORKFLOW_ID);
-        expect(vi.mocked(refactor).mock.calls[0]![2]).toBeTruthy();
+        expect(vi.mocked(refactor).mock.calls[0]![3]).toBeTruthy();
         // but didn't follow up with executing the action because we need to confirm
         expect(vi.mocked(refactor).mock.calls.length).toBe(1);
 

client/src/components/Workflow/Editor/RefactorConfirmationModal.vue

@@ -1,12 +1,17 @@
 <script setup lang="ts">
 import { ref, watch } from "vue";
 
-import { refactor } from "./modules/services";
+import {
+    refactor,
+    type RefactorRequestAction,
+    type RefactorResponse,
+    type RefactorResponseActionExecution,
+} from "@/api/workflows";
 
 import GModal from "@/components/BaseComponents/GModal.vue";
 
 interface Props {
-    refactorActions: any[]; // TODO: type from schema
+    refactorActions: RefactorRequestAction[];
     workflowId: string;
     title?: string;
     message?: string;
@@ -20,11 +25,11 @@ const emit = defineEmits<{
     (e: "onShow"): void;
     (e: "onWorkflowMessage", message: string, type: string): void;
     (e: "onWorkflowError", message: string, response: any): void;
-    (e: "onRefactor", data: any): void; // TODO: type from schema
+    (e: "onRefactor", data: RefactorResponse): void;
 }>();
 
 const show = ref(props.refactorActions.length > 0);
-const confirmActionExecutions = ref<any[]>([]); // TODO: type from schema
+const confirmActionExecutions = ref<RefactorResponseActionExecution[]>([]);
 
 watch(
     () => props.refactorActions,
@@ -46,21 +51,23 @@ watch(show, (newShow) => {
 async function dryRun() {
     emit("onWorkflowMessage", "Pre-checking requested workflow changes (dry run)...", "progress");
     try {
-        const data = await refactor(props.workflowId, props.refactorActions, true); // dry run
+        const data = await refactor(props.workflowId, props.refactorActions, "editor", true);
         onDryRunResponse(data);
     } catch (response) {
-        onError(response);
+        onError(response as string);
     }
 }
 
-function onError(response: any) {
+function onError(response: string) {
     emit("onWorkflowError", "Reworking workflow failed...", response);
 }
 
-function onDryRunResponse(data: any) {
+function onDryRunResponse(data: RefactorResponse) {
     // TODO: type from schema
     const actionExecutions = data.action_executions;
-    const anyRequireConfirmation = actionExecutions.some((execution: any) => execution.messages.length > 0);
+    const anyRequireConfirmation = actionExecutions.some(
+        (execution: RefactorResponseActionExecution) => execution.messages.length > 0,
+    );
     if (anyRequireConfirmation) {
         confirmActionExecutions.value = actionExecutions;
         show.value = true;
@@ -73,10 +80,10 @@ async function executeRefactoring() {
     show.value = false;
     emit("onWorkflowMessage", "Applying requested workflow changes...", "progress");
     try {
-        const data = await refactor(props.workflowId, props.refactorActions, false);
+        const data = await refactor(props.workflowId, props.refactorActions, "editor", false);
         emit("onRefactor", data);
     } catch (response) {
-        onError(response);
+        onError(response as string);
     }
 }
 </script>

client/src/components/Workflow/Editor/modules/linting.ts

@@ -5,20 +5,19 @@ import type { Steps } from "@/stores/workflowStepStore";
 import { assertDefined } from "@/utils/assertions";
 
 import { isWorkflowInput } from "../../constants";
+import type {
+    DisconnectedInputState,
+    DuplicateLabelState,
+    ExtractInputAction,
+    ExtractUntypedParameter,
+    LintState,
+    MetadataLintState,
+    RemoveUnlabeledWorkflowOutputs,
+    UnlabeledOuputState,
+    UntypedParameterState,
+} from "./lintingTypes";
 import { terminalFactory } from "./terminals";
 
-export interface LintState {
-    stepId: number;
-    stepLabel: string;
-    warningLabel: string;
-    name?: string;
-    inputName?: string;
-    autofix?: boolean;
-    data?: Record<string, string>;
-    /** Whether to highlight an input or output terminal */
-    highlightType?: "input" | "output";
-}
-
 export const bestPracticeWarningAnnotation =
     "This workflow does not provide a short description. Providing a short description helps workflow executors understand the purpose and usage of the workflow.";
 export const bestPracticeWarningAnnotationLength =
@@ -35,7 +34,7 @@ export function getDisconnectedInputs(
     datatypesMapper: DatatypesMapperModel,
     stores: ReturnType<typeof useWorkflowStores>,
 ) {
-    const inputs: LintState[] = [];
+    const inputs: DisconnectedInputState[] = [];
     Object.values(steps).forEach((step) => {
         step.inputs.map((inputSource) => {
             const inputTerminal = terminalFactory(step.id, inputSource, datatypesMapper, stores);
@@ -57,13 +56,13 @@ export function getDisconnectedInputs(
 }
 
 export function getMissingMetadata(steps: Steps) {
-    const inputs: LintState[] = [];
+    const inputs: MetadataLintState[] = [];
     Object.values(steps).forEach((step) => {
         if (isWorkflowInput(step.type)) {
             const noAnnotation = !step.annotation;
             const noLabel = !step.label;
             let warningLabel = null;
-            const data = {
+            const data: MetadataLintState["data"] = {
                 "missing-label": "false",
                 "missing-annotation": "false",
             };
@@ -91,17 +90,21 @@ export function getMissingMetadata(steps: Steps) {
     return inputs;
 }
 
-export function dataAttributes(action: LintState): Record<string, string> {
-    const result: Record<string, string> = {};
-    for (const [key, value] of Object.entries(action.data || {})) {
-        result[`data-${key}`] = value;
+// TODO: Maybe type of action should already be `MetadataLintState`?
+export function dataAttributes(action: LintState) {
+    const result: Record<string, "true" | "false"> = {};
+    // Ensure we have an attributes lint state (`MetadataLintState` with data)
+    if (isMetadataLintState(action)) {
+        for (const [key, value] of Object.entries(action.data)) {
+            result[`data-${key}`] = value;
+        }
     }
 
     return result;
 }
 
 export function getDuplicateLabels(steps: Steps, stores: ReturnType<typeof useWorkflowStores>) {
-    const duplicates: LintState[] = [];
+    const duplicates: DuplicateLabelState[] = [];
 
     const { stepStore } = stores;
     const labels = stepStore.duplicateLabels;
@@ -127,7 +130,7 @@ export function getDuplicateLabels(steps: Steps, stores: ReturnType<typeof useWo
 }
 
 export function getUnlabeledOutputs(steps: Steps) {
-    const outputs: LintState[] = [];
+    const outputs: UnlabeledOuputState[] = [];
     Object.values(steps).forEach((step) => {
         if (isWorkflowInput(step.type)) {
             // For now skip these... maybe should push this logic into linting though
@@ -153,7 +156,7 @@ export function getUnlabeledOutputs(steps: Steps) {
 }
 
 export function getUntypedParameters(untypedParameters: UntypedParameters) {
-    const items: LintState[] = [];
+    const items: UntypedParameterState[] = [];
     if (untypedParameters) {
         untypedParameters.parameters.forEach((parameter) => {
             try {
@@ -179,9 +182,9 @@ export function getUntypedParameters(untypedParameters: UntypedParameters) {
 }
 
 export function fixAllIssues(
-    untypedParameters: LintState[],
-    disconnectedInputs: LintState[],
-    unlabeledOutputs: LintState[],
+    untypedParameters: UntypedParameterState[],
+    disconnectedInputs: DisconnectedInputState[],
+    unlabeledOutputs: UnlabeledOuputState[],
 ) {
     const actions = [];
     for (const untypedParameter of untypedParameters) {
@@ -200,14 +203,14 @@ export function fixAllIssues(
     return actions;
 }
 
-export function fixUntypedParameter(untypedParameter: LintState) {
+export function fixUntypedParameter(untypedParameter: UntypedParameterState): ExtractUntypedParameter {
     return {
         action_type: "extract_untyped_parameter",
         name: untypedParameter.name,
     };
 }
 
-export function fixDisconnectedInput(disconnectedInput: LintState) {
+export function fixDisconnectedInput(disconnectedInput: DisconnectedInputState): ExtractInputAction {
     return {
         action_type: "extract_input",
         input: {
@@ -217,6 +220,21 @@ export function fixDisconnectedInput(disconnectedInput: LintState) {
     };
 }
 
-export function fixUnlabeledOutputs() {
+export function fixUnlabeledOutputs(): RemoveUnlabeledWorkflowOutputs {
     return { action_type: "remove_unlabeled_workflow_outputs" };
 }
+
+export function isDisconnectedInputState(state: LintState): state is DisconnectedInputState {
+    return isStateForInputOrOutput(state) && state.highlightType === "input";
+}
+
+function isMetadataLintState(state: LintState): state is MetadataLintState {
+    return "data" in state;
+}
+
+/** Type guard for linting states that are for a workflow step input or output. */
+export function isStateForInputOrOutput(
+    state: LintState,
+): state is DisconnectedInputState | DuplicateLabelState | UnlabeledOuputState {
+    return "highlightType" in state;
+}