Add Warning Modal for Expansion on Constraint Violation by pranav-super · Pull Request #1956 · NASA-AMMOS/plandev-ui

pranav-super · 2026-06-17T21:02:13Z

___REQUIRES_AERIE_PR___="1837"

This PR introduces a warning modal when a planner attempts to expand a sequence while there are still outstanding constraint violations. This feature was introduced based on a request by LRO mission operators.

Generally, the idea is to only warn users, but not to block anything. As constraint status is slightly nuanced, the following conditions are considered.

If simulation is out of date (i.e. the plan has been modified), the planner will be warned that their constraints may be outdated, and that expansion results could be misleading, as expansion will be done on the previous simulation, which may not match the current plan's status.
- There is an additional modification made to the expansion disabling functionality, stating that if the simulation is out of date, expansion is disabled. This can be undone, but was proposed in ExpansionPanel.svelte.
If constraint evaluation failed (not a violation, but failure to even evaluate), or incomplete, the planner is similarly warned.
If there are unchecked (or disabled, as PlanDev treats them the same in the UI) constraints, the planner should be warned that the current constraint evaluation may not be exhaustive.
If there are violated constraints, the planner should be warned that they are about to expand in spite of constraints that are failing.

Other statuses of constraints, like their being out of date (considered unchecked), partially complete, or anything else are not considered here.

Furthermore, the boolean argument bypassConstraints was added to calls to expandTemplates, in conjunction with an update to the backend which would throw errors on expansion if there were a violation and the bypass was not set. The default in the UI (and in the backend) is to not throw errors and bypass, as a warning is sufficient.

AaronPlave · 2026-06-30T14:47:20Z

Regarding this - any reason not to call these constraints out as being out of date vs unchecked? Complexity or is it hard to be certain of the status?

Other statuses of constraints, like their being out of date (considered unchecked), partially complete, or anything else are not considered here.

I think it was primarily to reduce the amount of information in this modal (and subsequently decrease implementation complexity). If this modal completely repeated all the information, it would be sort of redundant with respect to the existing constraints panel.

I was going for conciseness here, but does that seem reasonable?

I think we can probably punt on this. @pranav-super , your ticket captures this, right?

AaronPlave · 2026-06-30T14:52:11Z

Is it expected that if a constraint fails to compile (StayWellFed in this example below), it will not appear in the list of issues?

Now that I think about it, that might be worth mentioning as a failure of sorts! I say "of sorts" because while this is unexpected behavior and I agree with the implication that this probably should be presented in a warning, my question is about how concise we should be, and how to concisely present this!

@pranav-super sorry...I forgot if we talked about this specifically, was this covered by the "failing constraints"?

AaronPlave · 2026-06-30T15:00:19Z

    } else {
-      isExpansionDisabled = true;
-      expansionDisabledMessage = 'Completed simulation required';
+      // Question for PlanDev team: should we include this? Or only in the modal.


Any reason not to block before the modal? Is there any reason to open that modal before you have an up to date sim?

Also i seem to be able to open the modal and expand with an out of date sim

Ah - i see this happens only when i have no constraint violations, then I see the out of date sim message. Should stale sim be the first error precedence wise? Not totally sure, but should think on that.

What would all the cases be? Right now it's:

simulation is out of date -> not disabling because we can expand on an old simulation (?)

simulation is incomplete -> disabling, as we are trying to expand on the current simulation but cannot

no relevant sequences -> disabling, as we can't expand into anything

using templating -> not disabling...should still execute the functionality. This was something I left from before this PR

using sequencing -> depends; this logic was something I left from before this PR

@pranav-super Do you mind moving this to your ticket or a new one so that we know to revisit this later?

AaronPlave · 2026-06-30T15:02:41Z

+        : $checkConstraintsStatus === Status.Incomplete
+          ? ' (incomplete evaluation)'
+          : uncheckedConstraints.length > 0 || failingConstraints.length > 0
+            ? ` (${uncheckedConstraints.length > 0} unchecked, ${failingConstraints.length > 0} failed)`


want length not the boolean here for uncheckedConstraints.length > 0

should failingConstraints.length > 0 also just be failingConstraints.length?

AaronPlave · 2026-06-30T15:08:51Z

Have some language suggestions here:

Titles:

Failed -> "Constraints could not be evaluated"

Incomplete -> "Constraint evaluation incomplete"

Out of date -> "Simulation is out of date"

Unchecked -> "Unchecked constraints"

Violations -> "Constraint violations"

Body, by state:

Failed: "The most recent constraint evaluation failed, so the current constraint status is unknown."

Incomplete: "The most recent constraint evaluation didn't finish, so the current constraint status is incomplete."

Out of date: "The plan has changed since the last simulation, so the constraint results in the Constraints panel no longer reflect the current plan."

The constraint lists - can we render them as actual bulleted lists rather than stacked paragraphs and when the results are stale (eval failed/incomplete or sim out of date) preface them so it's clear they're from the last run:

Unchecked: "These constraints haven't been checked and may be violated:" (or "From the last evaluation, these constraints haven't been checked and may be violated:" when stale)

Violated: "These constraints are currently violated:" (or "The last evaluation found violations in these constraints:" when stale), with each entry pluralized properly — "(1 violation)" / "(2 violations)".

Closing line - one bolded statement of the actual risk, then the question:

For the out-of-date case: "Expansion will run against the previous simulation and may not match the current plan."

Otherwise: "Expanding now may produce sequences that violate mission constraints."

Then: "Do you want to expand anyway?"

I'd also relabel the "Expand" primary button in the modal to be "Expand anyway" and I'd make it destructive red (matching ConfirmModal).

This update has been made! The existing verbiage has been updated, though some of the other unreachable states you mentioned in other comments haven't been inserted.

Not sure if there was an offline agreement, but the Violations case says "Violating Constraints", instead of the suggested "Constraint violations".

AaronPlave · 2026-06-30T16:15:33Z

+        if (SEQUENCE_EXPANSION_MODE === SequencingMode.TEMPLATING) {
+          effects.expandTemplates([sequence.seq_id], $simulationDatasetLatest.id, $plan, user);
+        } else if (selectedExpansionSetId !== null) {
+          effects.expand(selectedExpansionSetId, $simulationDatasetLatest.id, $plan, user);


Does effects.expand need this bypassConstraints flag wired in as well?

Bigger point - why is this only applied to template expansion?

bypassConstraints is only really applicable/used when expansion is attempted without the UI. We wanted some way of allowing the backend to also pose this prevention (quick link to the PR), and since that endpoint is what expansion invokes, added the boolean there. It would always be set to true though, as we only want a warning modal and then no other protections. But if a CLI was implemented where they wanted that block, it would be possible to do so. I guess we could remove it altogether from effects...

The logic of only applying things to template expansion is not new to this PR though, but I'm not entirely sure what you mean!

I think that if the intent is that the UI shouldn't use it, then we shouldn't have it as an option to avoid confusion later on.

AaronPlave · 2026-06-30T16:16:50Z

+    }
+  }
+
+  // copied from ConstraintsPanel. Unable to move this into a store, as ConstraintsPanel directly modifies startTime, though we do not need to here.


Could we move this logic into a util that takes (specs, responseMap, startMs, endMs)?

I did try extracting it into a store earlier, but I believe I ran into an issue with startTime being modified in the ConstraintsPanel equivalent (though that is something we wouldn't want here). I think the same issue would arise in extracting it to a utility function!

I updated the comment to clarify that this is modified from ConstraintsPanel, not a direct copy. However, would it be worth breaking this into functions and reorganizing within ExpansionPanel itself?

I don't think it necessarily needs to be in a store. Stores should really only be for components that share the same values. I think what Aaron was asking was to, instead of copying the code and commenting where it came from, write a utility function that can be shared with both ExpansionPanel and ConstraintsPanel. It doesn't look like the code writes to any stores, just reads from them, so it shouldn't affect other panels.

AaronPlave · 2026-06-30T16:23:12Z

+    });
+  }
+
+  let allConstraintsHaveBeenChecked = true;


Can these lets be declared at the top of the file with the other lets? Also don't we need to reset these two allConstraints... booleans back to true at the start of the reactive block below since the &&= will only update these two variables if the left hand side is truthy?

@pranav-super did we confirm if the allConstraints... booleans should be reset to true at the start of the reactive block?

github-advanced-security · 2026-07-01T01:19:12Z

You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool.

What Enabling Code Scanning Means:

The 'Security' tab will display more code scanning analysis results (e.g., for the default branch).
Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results.
You will be able to see the analysis results for the pull request's branch on this overview once the scans have completed and the checks have passed.

For more information about GitHub Code Scanning, check out the documentation.

…junction with PlanDev base update

sonarqubecloud · 2026-07-09T21:17:54Z

Quality Gate failed

Failed conditions
15.8% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

duranb

The plan-expansion.test.ts is failing because it assumes that at least 2 expansion sets have been created prior to the test running. I haven't been able to find a separate test that creates these expansion sets, so I think that's why the tests are failing. Can you add the extra expansion set setup to the beginning of your test?

duranb · 2026-07-16T22:16:52Z

+  }
+  $: startTimeMs = typeof startTime === 'string' ? $plugins.time.primary.parse(startTime)?.getTime() : null;
+  $: endTimeMs = typeof endTime === 'string' ? $plugins.time.primary.parse(endTime)?.getTime() : null;
+  let constraintToConstraintResponseMap: ConstraintInvocationMap<ConstraintResponse> = {};


Do you mind moving this let up to the top where other lets are declared?

duranb · 2026-07-16T22:20:02Z

+    });
+  }
+
+  let allConstraintsHaveBeenChecked = true;


@pranav-super did we confirm if the allConstraints... booleans should be reset to true at the start of the reactive block?

duranb · 2026-07-16T22:28:29Z

+    }
+  }
+
+  // copied from ConstraintsPanel. Unable to move this into a store, as ConstraintsPanel directly modifies startTime, though we do not need to here.


I don't think it necessarily needs to be in a store. Stores should really only be for components that share the same values. I think what Aaron was asking was to, instead of copying the code and commenting where it came from, write a utility function that can be shared with both ExpansionPanel and ConstraintsPanel. It doesn't look like the code writes to any stores, just reads from them, so it shouldn't affect other panels.

duranb · 2026-07-16T22:34:39Z

+        if (SEQUENCE_EXPANSION_MODE === SequencingMode.TEMPLATING) {
+          effects.expandTemplates([sequence.seq_id], $simulationDatasetLatest.id, $plan, user);
+        } else if (selectedExpansionSetId !== null) {
+          effects.expand(selectedExpansionSetId, $simulationDatasetLatest.id, $plan, user);


I think that if the intent is that the UI shouldn't use it, then we shouldn't have it as an option to avoid confusion later on.

duranb · 2026-07-16T22:36:17Z

-      } else if (selectedExpansionSetId !== null) {
-        effects.expand(selectedExpansionSetId, $simulationDatasetLatest.id, $plan, user);
+  async function onExpandSequence(sequence: ExpansionSequence) {
+    var result = { confirm: true };


Can you change this to let result = { confirm: true }

duranb · 2026-07-16T22:44:19Z

Not sure if there was an offline agreement, but the Violations case says "Violating Constraints", instead of the suggested "Constraint violations".

duranb · 2026-07-16T22:48:13Z

    } else {
-      isExpansionDisabled = true;
-      expansionDisabledMessage = 'Completed simulation required';
+      // Question for PlanDev team: should we include this? Or only in the modal.


@pranav-super Do you mind moving this to your ticket or a new one so that we know to revisit this later?

duranb · 2026-07-16T22:50:28Z

@pranav-super sorry...I forgot if we talked about this specifically, was this covered by the "failing constraints"?

duranb · 2026-07-16T22:51:22Z

I think we can probably punt on this. @pranav-super , your ticket captures this, right?

duranb · 2026-07-16T22:59:42Z

+
+    // expand
+    await setup.plan.showPanel(PanelNames.EXPANSION);
+    await setup.page.locator('select[name="expansionSetId"]').selectOption('2');


I suggest avoiding using values unless we're absolutely sure that an expansion set with the ID "2" exists by the time this test is run. Even then, looking for a value that the test doesn't control can make it frail. It'd be more robust if you can invoke it like selectOption({ label: "[MY EXPANSION SET NAME]" }). That way the name is something we can decide within the test.

pranav-super requested a review from a team as a code owner June 17, 2026 21:02

pranav-super requested review from AaronPlave, joswig and mattdailis June 17, 2026 21:02

pranav-super had a problem deploying to test-workflow June 17, 2026 21:02 — with GitHub Actions Failure

pranav-super mentioned this pull request Jun 17, 2026

Optionally Disable Expansion if Constraints Violated NASA-AMMOS/plandev#1837

Open

pranav-super had a problem deploying to test-workflow June 17, 2026 21:07 — with GitHub Actions Failure

pranav-super had a problem deploying to test-workflow June 17, 2026 21:27 — with GitHub Actions Failure

AaronPlave added the feature New feature or request label Jun 22, 2026

pranav-super had a problem deploying to test-workflow June 24, 2026 21:55 — with GitHub Actions Failure

pranav-super had a problem deploying to test-workflow June 26, 2026 21:34 — with GitHub Actions Failure

pranav-super had a problem deploying to test-workflow June 26, 2026 21:39 — with GitHub Actions Failure

AaronPlave had a problem deploying to test-workflow June 26, 2026 22:06 — with GitHub Actions Failure