Initial `ModInstance` type and adapter applied to re-/activation code (1/3) #9182

twschiller · 2024-09-22T01:43:22Z

What does this PR do?

Context: https://www.notion.so/pixiebrix/Rearchitecting-Local-Representation-of-Activated-Mods-79012dd573ab47adaea959618e7b27e7?pvs=4
Adds ModInstance type
Add adapters to use ModInstance to simplify some React code:
- useActivateMod
- useReactivateMod
- useActivateModWizard
Floss refactoring:
- Removes the toggled mod component tracking from the WizardValues because we don't support toggling on/off individual mod components anymore

Discussion

By using adapters, we can evaluate the ergonomics of the shape before migrating the persisted mod component slice shape
Using the mod instance selector in the lifecycle is a bit tricky due to draft behavior:

pixiebrix-extension/src/contentScript/lifecycle.ts

Line 442 in 955553c

const modComponentsToActivate = options.activatedModComponents.filter(
- We may want to punt on that until we change Page Editor behavior to use drafts for all mod components when a mod is selected vs. only drafts for mod components that have been interacted with in the Page Editor
A benefit of doing this work now is that it simplifies some of the deployment code that @fungairino is doing

Future Work

Prioritizing changes that simplify deployment code, doesn't require a migration, and avoids the Page Editor:

Rewrite deployment updater to pass around ModInstance[] instead of ActivatedModComponent[]: Use ModInstance type in mod deployment code (2/3) #9190
Rewrite Mods Screen / Launcher to pass around ModInstance[] instead of passing around ActivatedModComponent[]. E.g., see buildModsList, buildGetModActivationStatus, etc.: Use ModInstance type on the mods screen (3/3) #9191

Consider adding a wrapper that wraps ModInstance with helper methods for derived data/properties. E.g., modId, isPaused, etc. without duplicating the data

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

twschiller · 2024-09-22T01:53:03Z

src/activation/useActivateModWizard.ts

-
-  const activatedOptions = collectModOptions(activatedModComponentsForMod);
-  const activatedIntegrationConfigs = Object.fromEntries(
-    collectConfiguredIntegrationDependencies(activatedModComponentsForMod).map(


TODO: see if we still need to filter activatedIntegrationConfigs to exclude the PixieBrix integration and unconfigured dependencies

twschiller · 2024-09-22T01:54:00Z

src/pageEditor/panes/save/saveHelpers.test.ts

@@ -890,115 +883,3 @@ describe("buildNewMod", () => {
    },
  );
 });
-
-describe("findMaxIntegrationDependencyApiVersion", () => {


Moved to utility folder test file

github-actions · 2024-09-22T02:18:51Z

Playwright test results

128 passed
2 flaky
4 skipped

Details

Open report ↗︎
134 tests across 44 suites
12 minutes, 44 seconds
4e69887
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

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

twschiller · 2024-09-22T02:44:27Z

src/activation/useActivateModWizard.ts

-  const hasPersonalDeployment = activatedModComponentsForMod?.some(
-    (x) => x._deployment?.isPersonalDeployment,
-  );
+  const hasPersonalDeployment =


Example of simplifying personal deployment code

twschiller · 2024-09-22T02:45:46Z

src/types/modInstanceTypes.ts

+   * In the future, we might consider eliminating by using a predictable id based on the mod instance id and position
+   * in the mod definition. But that's not possible today because the ids use a UUID format.
+   */
+  modComponentIds: UUID[];


A key insight I didn't originally anticipate is that ModInstance has to track the mod component ids. (See comment in code for why)

codecov · 2024-09-22T13:14:09Z

Codecov Report

Attention: Patch coverage is 91.89189% with 12 lines in your changes missing coverage. Please review.

Project coverage is 74.81%. Comparing base (8318d74) to head (4e69887).
Report is 306 commits behind head on main.

Files with missing lines	Patch %	Lines
src/activation/useActivateModWizard.ts	92.10%	3 Missing ⚠️
src/store/modComponents/modComponentUtils.ts	90.62%	3 Missing ⚠️
src/store/modComponents/modInstanceSelectors.ts	85.71%	2 Missing ⚠️
src/utils/registryUtils.ts	33.33%	2 Missing ⚠️
.../sidebar/activateMod/ActivateMultipleModsPanel.tsx	66.66%	1 Missing ⚠️
src/store/modComponents/modInstanceUtils.ts	96.42%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9182      +/-   ##
==========================================
+ Coverage   74.24%   74.81%   +0.56%     
==========================================
  Files        1332     1363      +31     
  Lines       40817    41955    +1138     
  Branches     7634     7830     +196     
==========================================
+ Hits        30306    31387    +1081     
- Misses      10511    10568      +57

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

twschiller · 2024-09-22T18:11:45Z

src/pageEditor/panes/save/saveHelpers.ts

- * @since 1.7.37
- * @note This function is just for safety, there's currently no way for a mod to end up with "mixed" integration api versions.
- */
-export function findMaxIntegrationDependencyApiVersion(


Moved the methods out of page editor directory to where they can be used cross-context

twschiller · 2024-09-22T18:12:49Z

src/utils/registryUtils.ts

+/**
+ * Returns a sharing object private package defined in the user's scope.
+ */
+export function createPrivateSharing(): Sharing {


There's a sharing factory. But in general, I think we want to keep prod data creation separate from test factories to avoid dummy data accidentally finding its way into production code

It would be good to align on naming conventions to distinguish, e.g., using create prefix in prod code vs. factory suffix

twschiller · 2024-09-22T18:21:42Z

src/activation/useActivateMod.test.ts

@@ -62,7 +62,6 @@ function setupInputs(): {
  modDefinition: ModDefinition;
 } {
  const formValues: WizardValues = {
-    modComponents: { 0: true },


Dropped unused modComponents toggle field from the type

twschiller · 2024-09-22T18:22:24Z

src/activation/useActivateModWizard.ts

 export type UseActivateModWizardResult = {
  wizardSteps: WizardStep[];
  initialValues: WizardValues;
  validationSchema: Yup.AnyObjectSchema;
 };

+function getInitialIntegrationDependencies(


Extracted these 2 from other method because they're both quite long

twschiller · 2024-09-22T18:22:54Z

src/extensionConsole/pages/activateMod/ActivateModPage.tsx

@@ -60,8 +60,6 @@ function useModNotFoundRedirectEffect(error: unknown): void {

 /**
 * Common page for activating a mod definition
- *
- * @param modDefinitionQuery The mod definition to activate


Invalid documentation

twschiller · 2024-09-22T18:24:00Z

src/store/modComponents/modInstanceUtils.ts

+ * Maps activated mod components to a mod instance.
+ * @param modComponents mod components from the mod
+ */
+export function mapActivatedModComponentsToModInstance(


The primary adapter to review in this PR

twschiller · 2024-09-22T18:24:51Z

src/store/modComponents/modInstanceUtils.ts

+      // definition vs. looking up the definition
+      options: emptyModOptionsDefinitionFactory(),
+      sharing: modMetadata.sharing ?? createPrivateSharing(),
+      updated_at: modMetadata.updated_at ?? firstComponent.updateTimestamp,


We might consider this line of refactoring as an opportunity to fix the snake case and/or perform other reshaping

twschiller · 2024-09-22T18:53:24Z

src/types/modInstanceTypes.ts

+   *
+   * NOTE: at this time, a device can only have one instance of a mod active at a time.
+   */
+  id: UUID;


Consider renaming to instanceId to better from mod registry id and ActivatedModComponent.id. Or consider tagging as an opaque type to prevent invalid assignments with ActivatedModComponent.id

twschiller added 2 commits September 21, 2024 21:30

[WIP] mod instance type and adapters

955553c

Remove some unrelated refactoring

dc76c27

twschiller self-assigned this Sep 22, 2024

vercel bot deployed to Preview September 22, 2024 01:49 View deployment

Fix bad refactor

005f8b0

vercel bot deployed to Preview September 22, 2024 01:52 View deployment

twschiller commented Sep 22, 2024

View reviewed changes

twschiller added the developer experience label Sep 22, 2024

twschiller changed the title ~~[WIP] mod instance type and adapters~~ [WIP] initial mod instance type and adapters Sep 22, 2024

twschiller requested a review from grahamlangford September 22, 2024 02:04

Refactor useReactivateMod

6a90319

vercel bot deployed to Preview September 22, 2024 02:31 View deployment

twschiller changed the title ~~[WIP] initial mod instance type and adapters~~ [WIP] initial mod instance type and adapter applies to activation code Sep 22, 2024

twschiller changed the title ~~[WIP] initial mod instance type and adapter applies to activation code~~ [WIP] initial mod instance type and adapter applied to re-/activation code Sep 22, 2024

twschiller commented Sep 22, 2024

View reviewed changes

twschiller added the 30% time label Sep 22, 2024

twschiller requested a review from fungairino September 22, 2024 12:18

Fix activation form validation

5f53bcc

vercel bot deployed to Preview September 22, 2024 13:08 View deployment

twschiller changed the title ~~[WIP] initial mod instance type and adapter applied to re-/activation code~~ Initial mod instance type and adapter applied to re-/activation code Sep 22, 2024

twschiller marked this pull request as ready for review September 22, 2024 13:09

twschiller changed the title ~~Initial mod instance type and adapter applied to re-/activation code~~ Initial mod instance type and adapter applied to re-/activation code (1/2) Sep 22, 2024

twschiller mentioned this pull request Sep 22, 2024

Use ModInstance type in mod deployment code (2/3) #9190

Open

1 task

Remove dead code

4e69887

vercel bot deployed to Preview September 22, 2024 14:47 View deployment

twschiller changed the title ~~Initial mod instance type and adapter applied to re-/activation code (1/2)~~ Initial mod instance type and adapter applied to re-/activation code (1/3) Sep 22, 2024

twschiller commented Sep 22, 2024

View reviewed changes

twschiller changed the title ~~Initial mod instance type and adapter applied to re-/activation code (1/3)~~ Initial ModInstance type and adapter applied to re-/activation code (1/3) Sep 22, 2024

twschiller commented Sep 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial `ModInstance` type and adapter applied to re-/activation code (1/3) #9182

Initial `ModInstance` type and adapter applied to re-/activation code (1/3) #9182

twschiller commented Sep 22, 2024 •

edited

Loading

twschiller Sep 22, 2024

twschiller Sep 22, 2024

github-actions bot commented Sep 22, 2024 •

edited

Loading

twschiller Sep 22, 2024

twschiller Sep 22, 2024 •

edited

Loading

codecov bot commented Sep 22, 2024 •

edited

Loading

twschiller Sep 22, 2024

twschiller Sep 22, 2024 •

edited

Loading

twschiller Sep 22, 2024

twschiller Sep 22, 2024

twschiller Sep 22, 2024

twschiller Sep 22, 2024

twschiller Sep 22, 2024

twschiller Sep 22, 2024 •

edited

Loading

Initial ModInstance type and adapter applied to re-/activation code (1/3) #9182

Are you sure you want to change the base?

Initial ModInstance type and adapter applied to re-/activation code (1/3) #9182

Conversation

twschiller commented Sep 22, 2024 • edited Loading

What does this PR do?

Discussion

Future Work

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 22, 2024 • edited Loading

Playwright test results

Details

Choose a reason for hiding this comment

twschiller Sep 22, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Sep 22, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

twschiller Sep 22, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twschiller Sep 22, 2024 • edited Loading

Choose a reason for hiding this comment

Initial `ModInstance` type and adapter applied to re-/activation code (1/3) #9182

Initial `ModInstance` type and adapter applied to re-/activation code (1/3) #9182

twschiller commented Sep 22, 2024 •

edited

Loading

github-actions bot commented Sep 22, 2024 •

edited

Loading

twschiller Sep 22, 2024 •

edited

Loading

codecov bot commented Sep 22, 2024 •

edited

Loading

twschiller Sep 22, 2024 •

edited

Loading

twschiller Sep 22, 2024 •

edited

Loading