Prevent deletion of the last mod component in a mod and enforce editor slice invariants

[twschiller]

What does this PR do?

• :bug: The Delete and Move from mod mod component actions are now disabled for the last mod component in a mod. This avoids the bug where a mod "disappears" when you remove the last component for the mod
  • If we wanted to allow "Move from mod" we could show a confirmation modal that continuing will deactivate the mod. (We'd have to deactivate to handle the case where it's an activated mod with a single mod component)
• Adds editorInvariantMiddleware in DEBUG mode to verify that invariant. 😱 The selectGetCleanComponentsAndDirtyFormStatesForMod included deleted states in modComponentFormStates in the test setup
• Editor slice refactoring/cleanup:
  • Drops redundant selectNotDeletedModComponentFormStates selector. It will always just be the mod component form states, because deleted form states are placed on deletedModComponentFormStatesByModId
  • Drops selectFirstModComponentFormStateForActiveMod because it was only being used to calculate dirty mod metadata and there's already a selector for that purpose
  • Moves editor slice helpers that are only used in 1 place back into the slice for readability
  • Re-use selectors in createSelector definition where possible
  • Modify selectors to require mod id vs. producing an empty value when mod id is null

Reviewer Notes

• The thing I'm most unsure about is whether the invariant between deletedModComponentFormStatesByModId and modComponentFormStates actually held as expected in the past or if there are invalid persisted editor slice states in the wild

For more information on our expectations for the PR process, see the code review principles doc

[github-actions[bot]]

Playwright test results

 144 passed
 2 skipped

Details

 Open report ↗︎
 146 tests across 47 suites
 9 minutes, 23 seconds
 6088392
 For more information on how to debug and view this report, see our readme

Skipped tests

chrome › tests/runtime/googleSheetsIntegration.spec.ts › can activate a google spreadsheet mod with config options msedge › tests/runtime/googleSheetsIntegration.spec.ts › can activate a google spreadsheet mod with config options

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 83.91608% with 23 lines in your changes missing coverage. Please review.

Project coverage is 75.30%. Comparing base (8318d74) to head (6088392). Report is 428 commits behind head on main.

Files with missing lines	Patch %	Lines
...geEditor/modListingPanel/modals/CreateModModal.tsx	16.66%	10 Missing :warning:
...geEditor/store/editor/editorInvariantMiddleware.ts	69.23%	8 Missing :warning:
src/pageEditor/store/editor/editorSlice.ts	86.95%	3 Missing :warning:
src/pageEditor/store/store.ts	75.00%	1 Missing :warning:
src/store/editorStorage.ts	75.00%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9360      +/-   ##
==========================================
+ Coverage   74.24%   75.30%   +1.06%     
==========================================
  Files        1332     1374      +42     
  Lines       40817    41873    +1056     
  Branches     7634     7766     +132     
==========================================
+ Hits        30306    31534    +1228     
+ Misses      10511    10339     -172     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[github-actions[bot]]

No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack.

Do not edit this comment manually.

[grahamlangford]

If we wanted to allow "Move from mod" we could show a confirmation modal that continuing will deactivate the mod. (We'd have to deactivate to handle the case where it's an activated mod with a single mod component)

If it's an activated mod, we should delete the mod (and tell the user that continuing will deactivate and delete the mod).

I think we also want to allow moving from unsaved mods regardless (just make sure we clear any weird leftover state). I can see developers not realizing they need to use the mod menu to add a second mod component and instead using new mod + move from mod

[twschiller]

If it's an activated mod, we should delete the mod (and tell the user that continuing will deactivate and delete the mod). I think we also want to allow moving from unsaved mods regardless (just make sure we clear any weird leftover state). I can see developers not realizing they need to use the mod menu to add a second mod component and instead using new mod + move from mod

I agree with both of these. My main concern is the corner case where the user might lose their work if they had updated the mod metadata. So I'd generally be in favor of the confirmation modal