Resolve conditional steps in benchmark wizard by ravikiranvm · Pull Request #1961 · openops-cloud/openops

ravikiranvm · 2026-02-18T12:54:38Z

Additional Notes

linear · 2026-02-18T12:54:42Z

ravikiranvm · 2026-02-18T13:35:54Z

packages/server/api/src/app/benchmark/providers/aws/aws-conditional-step-resolver.ts

+}
+
+async function hasMultipleAccounts(_context: WizardContext): Promise<boolean> {
+  // TODO: Evaluate from connection (e.g. roles.length > 1) and return false when no connection in context. For now always show the accounts step.


Will implement this in the follow up PR

ravikiranvm · 2026-02-18T13:36:40Z

packages/server/api/src/app/benchmark/provider-adapter.ts

    method: string,
    context: WizardContext,
  ): Promise<BenchmarkWizardOption[]>;
+  evaluateConditional(when: string, context: WizardContext): Promise<boolean>;


im indecisive between evaluateConditional and evaluateConditionalStep

evaluateCondition or resolveCondition

ravikiranvm · 2026-02-18T13:38:16Z

packages/server/api/src/app/benchmark/wizard.service.ts

+  if (!skipToStepId) {
+    return nextStepIdFromConfig;
+  }
+  return resolveNextStepId(skipToStepId, config, providerAdapter, context);


This is recursive because what if the resolved step is a conditional step?
But I don't think it is a common scenario; I can change it after inputs.

ravikiranvm · 2026-02-18T13:39:03Z

packages/server/api/src/app/benchmark/wizard.service.ts

  step: WizardConfigStep,
-  request: BenchmarkWizardRequest,
-  projectId: string,
+  context: WizardContext,


instead we build the context and pass it as a whole

ravikiranvm · 2026-02-18T13:40:01Z

packages/server/api/test/unit/benchmark/providers/aws/aws-conditional-step-resolver.test.ts

+      projectId,
+      provider,
+    });
+    // Returns true until we implement real evaluation (see hasMultipleAccounts in aws-conditional-step-resolver).


Will modify it once the real evaluation functions is available in the next PR

Copilot

Pull request overview

Implements support for conditional wizard steps in the benchmark flow (OPS-3650), allowing provider-specific logic to decide whether a step should be shown and enabling conditional option sources / skip behavior.

Changes:

Add ProviderAdapter.evaluateConditional() and implement AWS conditional evaluation (hasMultipleAccounts).
Update wizard navigation resolution to asynchronously skip conditional steps based on evaluated conditions.
Update AWS wizard config to use conditional.onSuccess.optionsSource and conditional.onFailure.skipToStep, and add unit tests for AWS conditional resolver.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
packages/server/api/src/app/benchmark/wizard.service.ts	Adds async conditional-step resolution and conditional optionsSource selection.
packages/server/api/src/app/benchmark/provider-adapter.ts	Extends conditional schema and ProviderAdapter with `evaluateConditional`.
packages/server/api/src/app/benchmark/providers/aws/aws-conditional-step-resolver.ts	Adds AWS conditional evaluation entrypoint (stubbed for now).
packages/server/api/src/app/benchmark/providers/aws/index.ts	Wires AWS `evaluateConditional` into the provider adapter.
packages/server/api/src/app/benchmark/providers/aws/aws.json	Updates AWS wizard step conditionals and options source behavior.
packages/server/api/test/unit/benchmark/wizard.service.test.ts	Updates wizard service test adapter mock to include `evaluateConditional`.
packages/server/api/test/unit/benchmark/providers/aws/aws-conditional-step-resolver.test.ts	Adds unit tests for AWS conditional evaluation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-18T13:40:47Z

packages/server/api/test/unit/benchmark/wizard.service.test.ts

  beforeEach(() => {
    jest.clearAllMocks();
    mockResolveOptions.mockResolvedValue([]);
+    mockEvaluateConditional.mockResolvedValue(true);
  });


The updated conditional behavior isn’t covered by unit tests: mockEvaluateConditional is always set to resolve true, so there’s no assertion that a conditional step is actually skipped (e.g., when it resolves false and onFailure.skipToStep is set). Add a test case where mockEvaluateConditional returns false and verify that navigation skips step2 to step3 and that evaluateConditional is called with the expected when and context.

Copilot · 2026-02-18T13:40:48Z

packages/server/api/src/app/benchmark/wizard.service.ts

+  const stepsWithOptions = config.steps.filter(
+    (s) => s.optionsSource || s.conditional?.onSuccess?.optionsSource,
+  );
  const totalSteps = stepsWithOptions.length;
  const stepIndex =
    stepsWithOptions.findIndex((s) => s.id === stepToReturn.id) + 1;
  return { totalSteps, stepIndex };


getStepProgress counts steps based on presence of an optionsSource (including conditional.onSuccess.optionsSource), but it doesn’t account for conditionals that are evaluated false and skipped. This can make totalSteps/stepIndex incorrect when a conditional step is not shown. Consider computing progress based on the resolved navigation path for the current context (i.e., evaluate conditionals and exclude skipped steps from the count/index).

Copilot · 2026-02-18T13:40:48Z

packages/server/api/src/app/benchmark/wizard.service.ts

+  const skipToStepId = step.conditional.onFailure?.skipToStep;
+  if (!skipToStepId) {
+    return nextStepIdFromConfig;
+  }
+  return resolveNextStepId(skipToStepId, config, providerAdapter, context);
+}


resolveNextStepId recursively follows conditional.onFailure.skipToStep without any cycle/depth protection. A misconfigured wizard (e.g., A skips to B and B skips back to A) will cause infinite recursion/stack overflow. Add cycle detection (visited set) or a max-depth guard and throw a validation error when a loop is detected.

Copilot · 2026-02-18T13:40:48Z

packages/server/api/src/app/benchmark/wizard.service.ts

+  const optionsSource =
+    step.conditional?.onSuccess?.optionsSource ??
+    (step.optionsSource as WizardStepOptionsSource | undefined);


resolveOptions always prefers step.conditional.onSuccess.optionsSource even though the conditional may evaluate to false (and the step may still be shown if there’s no onFailure.skipToStep). This makes the meaning of onSuccess inconsistent and can return the wrong options source. Consider passing the evaluated conditional result into resolveOptions (or evaluating the conditional there) so the onSuccess options source is only used when the condition actually succeeds.

Suggested change

const optionsSource =

step.conditional?.onSuccess?.optionsSource ??

(step.optionsSource as WizardStepOptionsSource | undefined);

const optionsSource = step.optionsSource as

| WizardStepOptionsSource

| undefined;

Copilot · 2026-02-18T13:40:48Z

packages/server/api/src/app/benchmark/providers/aws/aws.json

      "title": "Which accounts should we include in the report?",
      "selectionType": "multi-select",
-      "optionsSource": { "type": "dynamic", "method": "getConnectionAccounts" },
+      "optionsSource": "",


optionsSource is set to an empty string, but optionsSource is expected to be an object (or omitted). This relies on falsy behavior and type assertions and may break any runtime validation/parsing of the wizard config. Prefer removing optionsSource entirely for this step (and relying on conditional.onSuccess.optionsSource), or set it to null and explicitly handle null in the loader/types.

Suggested change

"optionsSource": "",

bigfluffycookie · 2026-02-18T13:21:52Z

packages/server/api/src/app/benchmark/wizard.service.ts

+        context,
+      );
+      const nextStepDef = steps.find((s) => s.id === resolvedNextStepId);
      if (!nextStepDef) {


you already throw in getNextStepIdFromConfig

bigfluffycookie · 2026-02-18T13:22:35Z

packages/server/api/src/app/benchmark/providers/aws/aws.json

      "title": "Which accounts should we include in the report?",
      "selectionType": "multi-select",
-      "optionsSource": { "type": "dynamic", "method": "getConnectionAccounts" },
+      "optionsSource": "",


remove this, you should only have it on steps without the conditionals

I think we decided keep it empty string for conditional steps? But I can remove it

no, for conditional steps you have it inside the condition, for non conditional steps you have it outside

bigfluffycookie · 2026-02-18T13:23:38Z

packages/server/api/src/app/benchmark/wizard.service.ts

+  providerAdapter: ProviderAdapter,
+  context: WizardContext,
+): Promise<string | null> {
+  if (!nextStepIdFromConfig) {


why even allow to add in no next stepId? just make nextStepIdFromConfig: string mandatory

bigfluffycookie · 2026-02-18T13:50:05Z

packages/server/api/src/app/benchmark/wizard.service.ts


-  const nextStep = resolveNextStepId(stepToShow, config);
+  const nextStepIdFromConfig = getNextStepIdFromConfig(stepToShow, config);
+  const nextStep = await resolveNextStepId(


This is now wrong.

Imagine this:
User is on step1 and clicks next
you look up step1s nextStep which is → step2

step2 has a conditional → evaluates it → condition is FALSE → step2 should not be shown, you recursively check if step3 should be shown (this is correct in theory but honestly the recursion will just confuse us all more)
stepToShow= step3
nextStepId = step4

step2 has a conditional → evaluates it → condition is TRUE → step2 should be shown with its options
stepToShow = step2
nextStepId = step3

BUT what happens now is you go in resolveNextStepId() with step3.id, and you go through the whole loop. no matter whats the case if step2 conditon evaluates to false it means you HAVE to skip to the next step.

if the condition is TRUE, we show the step.
I've named the method as hasMultipleAccounts and the success path would be to resolve options for the step.

you are right, i wrote my comment the wrong we around, i edited to fix it. The point I made is still correct though

Like maybe write a test case with 4 steps, where step 2 evaluates to TRUE and step 3 evaluates to FALSE. See what you get back from this method matches what you expect.

I suspect you get {stepToShow: step2, nextStepId: step4 }

bigfluffycookie · 2026-02-18T13:50:24Z

packages/server/api/src/app/benchmark/wizard.service.ts

+  };

-  const { stepToShow, nextStep } = computeWizardStepResponse(
+  const { stepToShow, nextStep } = await computeWizardStepResponse(


im sorry i missed it earlier, but it led me to be confused in some places now.

in here nextStep seems to be nextStepID
whereas stepToShow is an actual step.

yeah, here nextStep is the ID. I'm using nextStep here to match the usage in json file. I will see if I can make is more clear to read.

bigfluffycookie · 2026-02-18T13:54:07Z

packages/server/api/src/app/benchmark/wizard.service.ts

+  return resolveNextStepId(skipToStepId, config, providerAdapter, context);
+}
+
+async function computeWizardStepResponse(


i think the problem is that we are doing 2 things here.

we navigate to the next step

we look ahead at whats the next step id for the response to the frontend

in one case nextStep is the next step to navigate to and then later the next step is the nextsteps.nextstep to return

bigfluffycookie · 2026-02-18T13:57:25Z

packages/server/api/src/app/benchmark/wizard.service.ts

+  }
+  const step = config.steps.find((s) => s.id === nextStepIdFromConfig);


this is a bit funny, its needed bc of the recursion probably but in essence what you do is

you get a stepId

you call getNextStepIdFromConfig - which gets the steps[stepId]

you throw if there is no step

you return the stepId

you call this function with the ID to then get the step AGAIN from steps

you throw again if there is no step

bigfluffycookie

Wrong approve

Whopsie

MarceloRGonc · 2026-02-18T14:38:20Z

packages/server/api/src/app/benchmark/provider-adapter.ts

+  onSuccess?: {
+    optionsSource?: WizardStepOptionsSource;
+  };
+  onFailure?: {
+    skipToStep?: string;
+  };


Aren't these fields mandatory?

MarceloRGonc · 2026-02-18T14:47:01Z

packages/server/api/src/app/benchmark/provider-adapter.ts

    method: string,
    context: WizardContext,
  ): Promise<BenchmarkWizardOption[]>;
+  evaluateConditional(when: string, context: WizardContext): Promise<boolean>;


evaluateCondition or resolveCondition

sonarqubecloud · 2026-02-19T07:25:27Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Resolve conditional steps in benchmark wizard

d114136

ravikiranvm and others added 3 commits February 18, 2026 18:25

Merge branch 'main' into ops-3650

68fcb86

Add test file for conditionals

bfa11ba

Merge branch 'main' into ops-3650

0b690f8

ravikiranvm marked this pull request as ready for review February 18, 2026 13:35

Copilot AI review requested due to automatic review settings February 18, 2026 13:35

ravikiranvm commented Feb 18, 2026

View reviewed changes

Copilot started reviewing on behalf of ravikiranvm February 18, 2026 13:35 View session

ravikiranvm commented Feb 18, 2026

View reviewed changes

Copilot AI reviewed Feb 18, 2026

View reviewed changes

ravikiranvm requested review from MarceloRGonc, bigfluffycookie and cezudas February 18, 2026 13:44

bigfluffycookie previously approved these changes Feb 18, 2026

View reviewed changes

bigfluffycookie reviewed Feb 18, 2026

View reviewed changes

MarceloRGonc reviewed Feb 18, 2026

View reviewed changes

Merge branch 'main' into ops-3650

62c5bf4

ravikiranvm marked this pull request as draft February 20, 2026 06:01

		}
		const step = config.steps.find((s) => s.id === nextStepIdFromConfig);

Comments

Conversation

ravikiranvm commented Feb 18, 2026

Additional Notes

Uh oh!

linear bot commented Feb 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ravikiranvm Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bigfluffycookie Feb 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ravikiranvm Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bigfluffycookie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Feb 19, 2026

Quality Gate passed

Uh oh!

Reviewers

ravikiranvm Feb 19, 2026 •

edited

Loading

bigfluffycookie Feb 18, 2026 •

edited

Loading

ravikiranvm Feb 19, 2026 •

edited

Loading