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:40:34Z

+  export let constraintToConstraintResponseMap: ConstraintInvocationMap<ConstraintResponse>;
+  export let simulationDatasetId: number;
+
+  // TODO: adjust dynamically?


Would be better to just set a more generous height and the modal scroll on overflow since I'm seeing this sort of thing. Don't think dynamic height is worth complexity since at some point you're going to have to scroll anyway once you reach the full screen width (if you have lots of constraint issues).

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?

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!

AaronPlave · 2026-06-30T14:55:42Z

+      <p>The following constraints were violated:</p>
+      <div class="constraints-list">
+        {#each failingConstraints as failing}
+          <p>{failing.name} <b>({failing.viols} violations)</b></p>


Can we properly pluralize violation(s)? Have a pluralize util that we use for this.

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

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

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.

AaronPlave · 2026-06-30T15:14:12Z

+
+  let warningMessage = '';
+
+  $: if ($checkConstraintsStatus === Status.Failed) {


Need to handle Incomplete and Canceled?

For incomplete + canceled it'll just fall through to "Violating Constraints"

Those cases shouldn't trigger this modal, owing to the condition in onExpandSequence in ExpansionPanel.svelte.

AaronPlave · 2026-06-30T15:15:54Z

      $seqIds: [String!]!,
      $modelId: Int!,
      $simulationDatasetId: Int!
+      $bypassConstraints: Bool


Bool -> Boolean

AaronPlave · 2026-06-30T16:14:35Z

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


Any reason not to just get the result of confirm modal within that if block and if confirm is false just return early? Wouldn't have to gate the following logic in if (result.confirm)...

I think this is related to the earlier question of what to throw the modal up for. I also think what you're saying makes sense, but the idea here was to only show this modal in a subset of relevant scenarios (constraint checking was unsuccessful, simulation is out of date, or constraints weren't checked/failed). In any other scenarios, we wouldn't show the modal, as doing so might imply that the modal needs to convey extra information that could be redundant with what's already in the panel. This way, we only warn when we really need to (expansion on violation), but allow expansion otherwise.

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!

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?

AaronPlave · 2026-06-30T16:17:04Z

                  placement: 'top',
                }}
              >
+                <!-- <button


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?

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

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

AaronPlave reviewed Jun 30, 2026

View reviewed changes

AaronPlave requested changes Jun 30, 2026

View reviewed changes

pranav-super had a problem deploying to test-workflow July 1, 2026 01:16 — with GitHub Actions Failure

pranav-super had a problem deploying to test-workflow July 3, 2026 02:12 — with GitHub Actions Failure

pranav-super had a problem deploying to test-workflow July 3, 2026 02:20 — with GitHub Actions Failure

pranav-super had a problem deploying to test-workflow July 3, 2026 02:24 — with GitHub Actions Failure

pranav-super had a problem deploying to test-workflow July 3, 2026 02:28 — with GitHub Actions Failure

pranav-super temporarily deployed to test-workflow July 3, 2026 02:40 — with GitHub Actions Inactive

psubram3 added 15 commits July 9, 2026 11:17

add effects.ts TODO about expansion error display

b498db5

add ConfirmExpansionModal

2d0b441

add ConfirmExpansionModal to modal.ts

6f9dc35

add ConfirmExpansionModal to ExpansionPanel.svelte

4b82cf6

add optional bypassConstraints boolean to expandTemplates call in con…

70afb12

…junction with PlanDev base update

add number of violations to modal

ebe83ec

handle incomplete constraint checking

c959699

add logging of bypass

467308c

pass simulation dataset id to modal

334b32a

minor cleanup

2f75651

static modal height

a81b40f

pluralize violations

9d8606a

use length not boolean on length

7c9c535

move let's

22f8902

update Bool -> Boolean

23a0fc3

pranav-super force-pushed the feature/disable-expansion-on-constraint-viol branch from 8f5637e to 23a0fc3 Compare July 9, 2026 18:17

pranav-super had a problem deploying to test-workflow July 9, 2026 18:17 — with GitHub Actions Failure

update formatting and verbiage

734c809

pranav-super had a problem deploying to test-workflow July 9, 2026 19:27 — with GitHub Actions Failure

psubram3 added 2 commits July 9, 2026 13:49

update constraint tests pt 1

5357f21

update constraint tests pt 2

60c2244

pranav-super had a problem deploying to test-workflow July 9, 2026 20:49 — with GitHub Actions Failure

add expansion test that shows modal

de3a63d

pranav-super had a problem deploying to test-workflow July 9, 2026 21:15 — with GitHub Actions Failure


		let warningMessage = '';

		$: if ($checkConstraintsStatus === Status.Failed) {

Uh oh!

Conversation

pranav-super commented Jun 17, 2026 • edited by AaronPlave Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

pranav-super Jul 3, 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!

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!

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!

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!

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!

github-advanced-security AI commented Jul 1, 2026

What Enabling Code Scanning Means:

Uh oh!

sonarqubecloud Bot commented Jul 9, 2026

Quality Gate failed

Uh oh!

Reviewers

Assignees

Labels

pranav-super commented Jun 17, 2026 •

edited by AaronPlave

Loading

pranav-super Jul 3, 2026 •

edited

Loading