#9378: decouple mod options args from form states to apply to all mod components #9379

twschiller · 2024-10-29T11:54:22Z

What does this PR do?

Fixes Mod Options Args not applied unless mod component has been selected #9378
Refactors optionsArgs to be tracked at the mod-level instead of on form states
Introduces a draftModState type with mod-level edits to pass to the runtime/analysis engine

Remaining Work

Finish migrations
Finish rewiring analyses
Add migration tests
Rebase with main to get #9376: fix options args focus/active subtab resetting #9377
Add E2E test that updating the options arg updates the value

Future Work

Eliminate duplication of modMetadata
Decouple variablesDefinition from form states
Consider if we want to create form states for all mod components in a mod automatically. It's less important if we hoist mod-level data. However, there'd still be a benefit for simplifying Page Editor code (e.g., availability detection, Mod List item components, etc.). The only downside I see is increased use of localStorage because a copy of the whole mod is always stored

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

github-actions · 2024-10-29T12:08:24Z

Playwright test results

140 passed
6 flaky
2 skipped

Details

Open report ↗︎
148 tests across 48 suites
10 minutes, 16 seconds
d36e25f
For more information on how to debug and view this report, see our readme

Flaky tests

msedge-setup › setup/unaffiliated.setup.ts › authenticate with unaffiliated user
msedge › tests/extensionConsole/activation.spec.ts › can activate a mod with built-in integration
chrome › tests/pageEditor/copyMod.spec.ts › run a copied mod with a built-in integration
chrome › tests/pageEditor/saveMod.spec.ts › shows error notification when updating a public mod without incrementing the version
chrome › tests/runtime/sidebar/sidebarActivation.spec.ts › does not redirect to non-pixiebrix domain
msedge › tests/telemetry/errors.spec.ts › can report an indexdb error to telemetry service

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

twschiller · 2024-10-29T12:43:48Z

src/pageEditor/store/editor/editorSlice.test.ts

-    expect(updatedExistingComponent?.optionsArgs).toEqual(updatedOptionsArgs);
-  });
-
-  test("deleted mod component form states don't have their options args updated", () => {


These tests are no longer relevant

twschiller · 2024-10-29T12:44:38Z

src/pageEditor/store/editor/pageEditorTypes.ts

+ * @deprecated - Do not use versioned state types directly, exported for testing
+ * @since 2.1.6
+ */
+export type EditorStateV10 = Except<


Type changes

src/pageEditor/tabs/modOptionsArgs/ModOptionsArgsEditor.tsx

Finish todo Fix more type errors [WIP] refactor analysis Fix types/tests

twschiller · 2024-10-29T14:51:16Z

src/pageEditor/tabs/modOptionsDefinitions/ModOptionsDefinitionEditor.tsx

 const ModOptionsDefinitionEditor: React.VFC = () => {
  const [activeField, setActiveField] = useState<string | null>(null);
  const modId = useSelector(selectActiveModId);
  assertNotNullish(modId, "Expected active mod id");

-  const {


Refactor to use AsyncStateGate

twschiller · 2024-10-29T14:51:54Z

src/store/editorMigrations.ts

+}
+
+/** @internal */
+export function migrateEditorStateV9(


@grahamlangford migration code here. Note the use of dirty to decide whether to set the dirty options args

codecov · 2024-10-29T15:27:45Z

Codecov Report

Attention: Patch coverage is 92.30769% with 8 lines in your changes missing coverage. Please review.

Project coverage is 75.33%. Comparing base (8318d74) to head (d36e25f).
Report is 434 commits behind head on main.

Files with missing lines	Patch %	Lines
src/pageEditor/starterBricks/quickBar.ts	0.00%	2 Missing ⚠️
...dOptionsDefinitions/ModOptionsDefinitionEditor.tsx	87.50%	2 Missing ⚠️
...or/hooks/useRegisterDraftModInstanceOnAllFrames.ts	75.00%	1 Missing ⚠️
src/pageEditor/starterBricks/contextMenu.ts	50.00%	1 Missing ⚠️
src/pageEditor/starterBricks/dynamicQuickBar.ts	0.00%	1 Missing ⚠️
src/store/editorMigrations.ts	92.85%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9379      +/-   ##
==========================================
+ Coverage   74.24%   75.33%   +1.08%     
==========================================
  Files        1332     1374      +42     
  Lines       40817    41884    +1067     
  Branches     7634     7764     +130     
==========================================
+ Hits        30306    31553    +1247     
+ Misses      10511    10331     -180

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

twschiller · 2024-10-29T15:48:50Z

src/pageEditor/store/editor/pageEditorTypes.ts

+ * Mod-level editor state passed to the runtime/analysis engine
+ * @since 2.1.6
+ */
+export type DraftModState = {


This DraftModState type will eventually contain the other mod-level information for the draft:

Mod Options Definitions

Mod Variable Definitions

Mod Metadata

github-actions · 2024-10-29T17:49:12Z

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.

twschiller changed the title ~~#9378: decouple mod options args and form state~~ #9378: decouple mod options args from form states to fix args bugs Oct 29, 2024

vercel bot deployed to Preview October 29, 2024 11:54 View deployment

twschiller added this to the 2.1.6 milestone Oct 29, 2024

twschiller added bug Something isn't working page editor labels Oct 29, 2024

twschiller changed the title ~~#9378: decouple mod options args from form states to fix args bugs~~ #9378: decouple mod options args from form states to apply to all mod components Oct 29, 2024

twschiller requested a review from grahamlangford October 29, 2024 12:02

twschiller changed the base branch from main to feature/9355-clear-changes October 29, 2024 12:42

twschiller added the do not merge Merging of this PR is blocked by another one label Oct 29, 2024

twschiller commented Oct 29, 2024

View reviewed changes

vercel bot deployed to Preview October 29, 2024 12:44 View deployment

twschiller commented Oct 29, 2024

View reviewed changes

vercel bot deployed to Preview October 29, 2024 12:58 View deployment

grahamlangford reviewed Oct 29, 2024

View reviewed changes

src/pageEditor/tabs/modOptionsArgs/ModOptionsArgsEditor.tsx Outdated Show resolved Hide resolved

twschiller self-assigned this Oct 29, 2024

twschiller added 2 commits October 29, 2024 10:28

[WIP]: options args

44e42f7

Finish todo Fix more type errors [WIP] refactor analysis Fix types/tests

Fix tests

a9c1a82

twschiller force-pushed the feature/mod-args branch from 0ad99cf to a9c1a82 Compare October 29, 2024 14:49

twschiller removed the do not merge Merging of this PR is blocked by another one label Oct 29, 2024

twschiller changed the base branch from feature/9355-clear-changes to main October 29, 2024 14:49

twschiller marked this pull request as ready for review October 29, 2024 14:50

vercel bot deployed to Preview October 29, 2024 14:51 View deployment

twschiller commented Oct 29, 2024

View reviewed changes

Fix passing args to draft runtime components

44d7dbf

vercel bot deployed to Preview October 29, 2024 15:14 View deployment

Move type

01fdf8c

vercel bot deployed to Preview October 29, 2024 15:21 View deployment

Add playwright test

23048c6

vercel bot deployed to Preview October 29, 2024 15:48 View deployment

twschiller commented Oct 29, 2024

View reviewed changes

Fix grammar

d36e25f

vercel bot deployed to Preview October 29, 2024 16:04 View deployment

grahamlangford approved these changes Oct 29, 2024

View reviewed changes

twschiller merged commit af74eea into main Oct 29, 2024
23 checks passed

twschiller deleted the feature/mod-args branch October 29, 2024 17:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#9378: decouple mod options args from form states to apply to all mod components #9379

#9378: decouple mod options args from form states to apply to all mod components #9379

twschiller commented Oct 29, 2024 •

edited

Loading

github-actions bot commented Oct 29, 2024 •

edited

Loading

twschiller Oct 29, 2024

twschiller Oct 29, 2024

twschiller Oct 29, 2024

twschiller Oct 29, 2024 •

edited

Loading

codecov bot commented Oct 29, 2024 •

edited

Loading

twschiller Oct 29, 2024 •

edited

Loading

github-actions bot commented Oct 29, 2024

#9378: decouple mod options args from form states to apply to all mod components #9379

#9378: decouple mod options args from form states to apply to all mod components #9379

Conversation

twschiller commented Oct 29, 2024 • edited Loading

What does this PR do?

Remaining Work

Future Work

github-actions bot commented Oct 29, 2024 • edited Loading

Playwright test results

Details

twschiller Oct 29, 2024

Choose a reason for hiding this comment

twschiller Oct 29, 2024

Choose a reason for hiding this comment

twschiller Oct 29, 2024

Choose a reason for hiding this comment

twschiller Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Oct 29, 2024 • edited Loading

Codecov Report

twschiller Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Oct 29, 2024

twschiller commented Oct 29, 2024 •

edited

Loading

github-actions bot commented Oct 29, 2024 •

edited

Loading

twschiller Oct 29, 2024 •

edited

Loading

codecov bot commented Oct 29, 2024 •

edited

Loading

twschiller Oct 29, 2024 •

edited

Loading