#9242: migrate unsaved standalone mod components in the Page Editor #9246

twschiller · 2024-10-07T01:01:30Z

What does this PR do?

Closes Slice 6: Migrate all remaining StandaloneModComponents to UnsavedModDefinitions or ModDefintions #9242
Closes Slice 7: Make the modMetadata field in BaseFormState required #9247
Stacked on #9244: add Move/Copy mod component actions #9245
Primary change: Migrates editorSlice form state to require modMetadata
Drops Page Editor actions that were specific to standalone unsaved mods
Refactoring:
- Drop unnecessary user of ? on form state modMetadata
- Refactor fromNativeElement to accept Mod Metadata

Reviewer Notes

Drops useMigrateStandaloneComponentsToMods. See decision/discussion here: #9242: migrate unsaved standalone mod components in the Page Editor #9246 (comment)
While the slice name / implementation sketch refers to the form states as corresponding to unsaved mods, note that's different than the UnsavedModDefinition type

Future Work

Migrate ModComponent type to require _recipe/modMetadata

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

twschiller · 2024-10-07T02:42:31Z

src/pageEditor/store/editor/editorSlice.ts

      );
      for (const formState of modComponentFormStates) {
        formState.modMetadata = modMetadata;
      }
    },
-    addModComponentFormStateToMod(


Drop unused action

twschiller · 2024-10-07T02:42:37Z

src/pageEditor/store/editor/editorSlice.ts

@@ -670,53 +628,6 @@ export const editorSlice = createSlice({
      state.visibleModalKey = ModalKey.MOVE_COPY_TO_MOD;
      state.keepLocalCopyOnCreateMod = moveOrCopy === "copy";
    },
-    removeModComponentFormStateFromMod(


Drop unused action

twschiller · 2024-10-07T02:43:04Z

src/pageEditor/store/editor/baseFormStateTypes.ts

+   * The mod metadata for the mod component
+   * @see createNewUnsavedModMetadata
+   */
+  modMetadata: ModMetadata;


Main type change: require modMetadata

Instead of Except, would SetRequired be better here?

Sure - I'll make the change. I think the end result is the same? I agree SetRequired is probably more readable

Looks like we can't use it since SetRequired seems to operate on "?" and not types where the prop exists but the value might be set to undefined

That makes sense now that I think about it. I need to poll with the team whether we want to start differentiating between ? and | undefined, since we've run into at least one bug due to the difference. There's a TS setting to enforce similar to strictNullChecks

IIRC, historically when adding a new prop, we've used foo: X | undefined so that the type checker forces us to think about where to provide a default value, etc. vs. accidentally leaving the property off

Agree that we might choose to revisit that decision, e.g., now that we have strict null checks enabled

twschiller · 2024-10-07T02:47:28Z

src/store/editorMigrations.ts

+export function migrateEditorStateV8(
+  state: EditorStateV8 & PersistedState,
+): EditorStateV9 & PersistedState {
+  return produce(state, (draft) => {


Slice migration code

grahamlangford · 2024-10-07T18:27:01Z

@grahamlangford I don't think I understand the sketch here: https://www.notion.so/pixiebrix/Re-Slicing-Eliminate-Standalone-mods-from-UI-694e4c85e61441809e14aec18f6b6109?pvs=4#10a43b21a253801a93e5e7e0cabb0b4d. UnsavedModDefinition is not how the Page Editor tracks mods/components. So there isn't really a way to convert standalone mod components to UnsavedModDefinition

If the user has any unsaved standalone mod components in their Page Editor, then we need to change them (structurally and visually) to match the new behavior when you create a new mod but don't save it via the dropdown (pushing metadata down with the @inner id, etc.

This is so we can get rid of the UI affordances for standalone mod components entirely

@grahamlangford do you remember the context behind why the sketch indicated we have to keep useMigrateStandaloneComponentsToMods? https://www.notion.so/pixiebrix/Re-Slicing-Eliminate-Standalone-mods-from-UI-694e4c85e61441809e14aec18f6b6109?pvs=4#10a43b21a25380e4974ae924d03f3fc5?. That hook loops over standalone mod component form states. So, if the Page Editor migrates those, the set of those would always be empty by the time it hits the hook

Sure, the idea is to be defensive:

I am on an older version of the Extension (either before useMigrateStandaloneComponentsToMods was added or if it's more recent, I haven't opened the Page Editor since before)
I have saved standalone mods that have been converted on the backend but not in the Extension. (because my previous version was too old or because I haven't opened the Page Editor in several versions
I update my Extension to 2.1.4
I open my Page Editor
I need my persisted standalone mod components to be converted or I will probably experience UI errors (depending on how we handle removing the affordances for standalone mod components)

grahamlangford · 2024-10-07T20:23:00Z

src/pageEditor/hooks/useMigrateStandaloneComponentsToMods.test.ts

+            modComponentActions.UNSAFE_setModComponents(
+              array(activatedModComponentFactory, 3)(),
+            ),


I forgot about that helper. There are so many places that would be useful

grahamlangford · 2024-10-07T20:24:48Z

src/pageEditor/hooks/useMigrateStandaloneComponentsToMods.ts

+// FIXME: why is this hook still required?
+//  See comment: https://www.notion.so/pixiebrix/Re-Slicing-Eliminate-Standalone-mods-from-UI-694e4c85e61441809e14aec18f6b6109?pvs=4#10a43b21a25380e4974ae924d03f3fc5


As noted on the PR comment, it's purely for backwards compatibility in case the user skipped multiple versions or didn't open the Page Editor in the time since the hook was added. We could consider other options as well (have redux-persist migrations delete any still-existing standalone mod components from both the options and editor stores?)

I think the hook is a NOP though given the migration in this PR. Given the migration, standaloneComponentFormStates will always be an empty array

I agree there's a corner case for someone on 1/ a version of PixieBrix without the mod component slice migration, 2/ an unsaved standalone mod form state. They could get into a case where a different mod id is assigned to the form state vs. the mod component, but the mod id component will be the same

Personally, I don't think we need to account for people with unsaved standalone mod component who haven't opened the Page Editor in 2+ releases. They can recover just by saving deleting the unsaved mod from the Page Editor and recreating it

That's a good point. Though in that case we could end up in a situation where the App converted a persisted standalone mod component into a mod definition and the Extension converted the same persisted standalone mod component into an UnsavedMod.

I'll leave it to you to determine how defensive we want to be. It might be that the correct thing to do in that case is to update the hook to find mod components that have a temporary registry id instead of nullish metadata instead of removing it

That's a good point. Though in that case we could end up in a situation where the App converted a persisted standalone mod component into a mod definition and the Extension converted the same persisted standalone mod component into an UnsavedMod.

Yep - that's the corner case.

I'll leave it to you to determine how defensive we want to be

My strong preference is to just drop the code given that it's over a month/2+ releases away, outside our enterprise support window, and a recoverable issue

src/pageEditor/modListingPanel/modals/MoveCopyToModModal.tsx

grahamlangford · 2024-10-07T20:52:34Z

src/pageEditor/starterBricks/button.ts

+  url,
+  modMetadata,
+  starterBrickMetadata,
+  element,


NIT: I agree that button is the wrong name for this, but perhaps selectionResult would read better?

It's using "element" because of the method name fromNativeElement. That's the "element" it's being created from. So if we wanted to use selectionResult, we'd probably want to rename the whole method in the adapter protocol

Leaving off of this PR

src/pageEditor/store/editor/editorSlice.ts

grahamlangford · 2024-10-07T21:06:40Z

src/pageEditor/store/editor/editorSlice.ts

-      const modComponentFormState =
-        action.payload as Draft<ModComponentFormState>;
+      // Ensure the form state is writeable for normalization
+      const modComponentFormState = cloneDeep(action.payload);


I'm curious if we should consider switching to structuredClone

🤷 Not sure. Initial Google search/chatter indicates structuredClone might be slower?

It might be. I haven't looked into any tests. I do know that structuredClone makes a copy of the prototype, whereas lodash cloneDeep apparently copies a reference to it. That's unlikely to ever be a problem for us though

Fixup for _recipe/modMetadata Revert cross-cutting refactoring Revert cross-cutting refactoring

github-actions · 2024-10-07T21:41:14Z

Playwright test results

135 passed
3 flaky
4 skipped

Details

Open report ↗︎
142 tests across 46 suites
13 minutes, 28 seconds
bbeb17e
For more information on how to debug and view this report, see our readme

Flaky tests

chrome › tests/pageEditor/addStarterBrick.spec.ts › Add starter brick to mod
msedge › tests/pageEditor/addStarterBrick.spec.ts › Add starter brick to mod
msedge › tests/regressions/welcomeStarterBricks.spec.ts › #8740: can view the starter mods on the pixiebrix.com/welcome page

Skipped tests

chrome › tests/regressions/doNotCloseSidebarOnPageEditorSave.spec.ts › #8104: Do not automatically close the sidebar when saving in the Page Editor
msedge › tests/regressions/doNotCloseSidebarOnPageEditorSave.spec.ts › #8104: Do not automatically close the sidebar when saving in the Page Editor
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 · 2024-10-08T01:22:14Z

Codecov Report

Attention: Patch coverage is 78.48101% with 17 lines in your changes missing coverage. Please review.

Project coverage is 75.10%. Comparing base (8318d74) to head (bbeb17e).
Report is 347 commits behind head on main.

Files with missing lines	Patch %	Lines
...itor/modListingPanel/DraftModComponentListItem.tsx	60.00%	4 Missing ⚠️
src/pageEditor/hooks/useAddNewModComponent.ts	0.00%	3 Missing ⚠️
src/pageEditor/starterBricks/base.ts	71.42%	2 Missing ⚠️
src/pageEditor/starterBricks/dynamicQuickBar.ts	0.00%	2 Missing ⚠️
src/pageEditor/hooks/useClearModChanges.ts	0.00%	1 Missing ⚠️
...geEditor/modListingPanel/modals/CreateModModal.tsx	0.00%	1 Missing ⚠️
src/pageEditor/store/analysisManager.ts	50.00%	1 Missing ⚠️
src/pageEditor/store/editor/editorSlice.ts	66.66%	1 Missing ⚠️
src/pageEditor/utils.ts	0.00%	1 Missing ⚠️
src/store/editorMigrations.ts	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9246      +/-   ##
==========================================
+ Coverage   74.24%   75.10%   +0.85%     
==========================================
  Files        1332     1370      +38     
  Lines       40817    42306    +1489     
  Branches     7634     7891     +257     
==========================================
+ Hits        30306    31773    +1467     
- Misses      10511    10533      +22

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

grahamlangford · 2024-10-08T13:34:03Z

src/pageEditor/store/editor/baseFormStateTypes.ts

+  // Can't use SetRequired because the property is required (it does not use ?), but it can be set to undefined
+  Except<BaseFormStateV5<TModComponent, TStarterBrick>, "modMetadata"> & {
+    /**
+     * The mod metadata for the mod component
+     * @see createNewUnsavedModMetadata
+     */
+    modMetadata: ModMetadata;
+  };


github-actions · 2024-10-08T13:42: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 added page editor developer experience labels Oct 7, 2024

twschiller self-assigned this Oct 7, 2024

twschiller requested review from grahamlangford and mnholtz October 7, 2024 01:19

twschiller changed the title ~~#9242: migrate unsaved standalone mod components in the Page Editor~~ #9242: [WIP] migrate unsaved standalone mod components in the Page Editor Oct 7, 2024

twschiller requested review from grahamlangford and removed request for grahamlangford and mnholtz October 7, 2024 01:20

twschiller commented Oct 7, 2024

View reviewed changes

vercel bot deployed to Preview October 7, 2024 02:42 View deployment

twschiller commented Oct 7, 2024

View reviewed changes

vercel bot deployed to Preview October 7, 2024 02:47 View deployment

twschiller commented Oct 7, 2024

View reviewed changes

twschiller added this to the 2.1.4 milestone Oct 7, 2024

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

twschiller force-pushed the feature/9244-move-copy-mod-component branch from 99f9601 to 9c49084 Compare October 7, 2024 18:27

grahamlangford reviewed Oct 7, 2024

View reviewed changes

src/pageEditor/modListingPanel/modals/MoveCopyToModModal.tsx Outdated Show resolved Hide resolved

grahamlangford reviewed Oct 7, 2024

View reviewed changes

src/pageEditor/store/editor/editorSlice.ts Show resolved Hide resolved

grahamlangford reviewed Oct 7, 2024

View reviewed changes

twschiller force-pushed the feature/9242-migrate-unsaved-standalone branch from 1040c91 to 666f093 Compare October 7, 2024 21:13

vercel bot deployed to Preview October 7, 2024 21:14 View deployment

Base automatically changed from feature/9244-move-copy-mod-component to main October 7, 2024 21:20

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

9242: migrate unsaved standalone mod components

accb448

Fixup for _recipe/modMetadata Revert cross-cutting refactoring Revert cross-cutting refactoring

twschiller force-pushed the feature/9242-migrate-unsaved-standalone branch from 666f093 to accb448 Compare October 7, 2024 21:22

vercel bot deployed to Preview October 7, 2024 21:23 View deployment

twschiller added 3 commits October 7, 2024 20:41

Drop useMigrateStandaloneComponentsToMods

699d59c

#9242: add migration test case

495c35e

Improve file naming

8ff757f

twschiller changed the title ~~#9242: [WIP] migrate unsaved standalone mod components in the Page Editor~~ #9242: migrate unsaved standalone mod components in the Page Editor Oct 8, 2024

vercel bot deployed to Preview October 8, 2024 01:16 View deployment

twschiller marked this pull request as ready for review October 8, 2024 01:16

grahamlangford reviewed Oct 8, 2024

View reviewed changes

Merge with main

bbeb17e

vercel bot deployed to Preview October 8, 2024 13:38 View deployment

grahamlangford approved these changes Oct 8, 2024

View reviewed changes

twschiller enabled auto-merge (squash) October 8, 2024 13:45

twschiller added the user experience Improve the user experience (UX) label Oct 8, 2024

twschiller merged commit 91ceceb into main Oct 8, 2024
32 checks passed

twschiller deleted the feature/9242-migrate-unsaved-standalone branch October 8, 2024 13:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#9242: migrate unsaved standalone mod components in the Page Editor #9246

#9242: migrate unsaved standalone mod components in the Page Editor #9246

twschiller commented Oct 7, 2024 •

edited

Loading

twschiller Oct 7, 2024

twschiller Oct 7, 2024

twschiller Oct 7, 2024

grahamlangford Oct 7, 2024

twschiller Oct 7, 2024 •

edited

Loading

twschiller Oct 8, 2024 •

edited

Loading

grahamlangford Oct 8, 2024

twschiller Oct 8, 2024

twschiller Oct 7, 2024

grahamlangford commented Oct 7, 2024 •

edited

Loading

grahamlangford Oct 7, 2024

grahamlangford Oct 7, 2024 •

edited

Loading

twschiller Oct 7, 2024 •

edited

Loading

grahamlangford Oct 7, 2024

twschiller Oct 8, 2024

grahamlangford Oct 7, 2024

twschiller Oct 7, 2024 •

edited

Loading

grahamlangford Oct 7, 2024

twschiller Oct 8, 2024 •

edited

Loading

grahamlangford Oct 8, 2024

github-actions bot commented Oct 7, 2024 •

edited

Loading

codecov bot commented Oct 8, 2024 •

edited

Loading

grahamlangford Oct 8, 2024

github-actions bot commented Oct 8, 2024

		// FIXME: why is this hook still required?
		// See comment: https://www.notion.so/pixiebrix/Re-Slicing-Eliminate-Standalone-mods-from-UI-694e4c85e61441809e14aec18f6b6109?pvs=4#10a43b21a25380e4974ae924d03f3fc5

#9242: migrate unsaved standalone mod components in the Page Editor #9246

#9242: migrate unsaved standalone mod components in the Page Editor #9246

Conversation

twschiller commented Oct 7, 2024 • edited Loading

What does this PR do?

Reviewer Notes

Future Work

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twschiller Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

twschiller Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grahamlangford commented Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

grahamlangford Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

twschiller Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twschiller Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twschiller Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Playwright test results

Details

codecov bot commented Oct 8, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

github-actions bot commented Oct 8, 2024

twschiller commented Oct 7, 2024 •

edited

Loading

twschiller Oct 7, 2024 •

edited

Loading

twschiller Oct 8, 2024 •

edited

Loading

grahamlangford commented Oct 7, 2024 •

edited

Loading

grahamlangford Oct 7, 2024 •

edited

Loading

twschiller Oct 7, 2024 •

edited

Loading

twschiller Oct 7, 2024 •

edited

Loading

twschiller Oct 8, 2024 •

edited

Loading

github-actions bot commented Oct 7, 2024 •

edited

Loading

codecov bot commented Oct 8, 2024 •

edited

Loading