Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#8131 Auto-deactivate unassigned mods on Mods Page #8164

Merged
merged 60 commits into from Apr 5, 2024

Conversation

mnholtz
Copy link
Collaborator

@mnholtz mnholtz commented Apr 5, 2024

What does this PR do?

Discussion

  • Tentatively put the deactivation logic inside the useAutoDeploy useEffect instead of creating a separate effect to put somewhere. Happy to extract that out into its own effect, though; it's something that I considered.

Demo

Todo

  • Write unit tests

Checklist

  • Add tests and/or storybook stories
  • Designate a primary reviewer @grahamlangford

deployments: deactivatedModComponents.map(
(modComponent) => modComponent._deployment.id,
),
});
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also mirrors logic in deploymentUpdater, see deactivateUnassignedDeployments. It's tricky to extract and share this logic with the current architecture because the deploymentUpdater can't dispatch redux actions.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deploymentUpdater can't dispatch redux actions.

Technically you can, you just have to manually call the reducer and use our set the resulting state using setReduxStorage

Copy link
Collaborator Author

@mnholtz mnholtz Apr 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically you can, you just have to manually call the reducer and use our set the resulting state using setReduxStorage

Right, what I mean is that having to do that makes logic sharing tricky between the background and the react code

@mnholtz mnholtz marked this pull request as ready for review April 5, 2024 01:59
@grahamlangford
Copy link
Collaborator

Tentatively put the deactivation logic inside the useAutoDeploy useEffect instead of creating a separate effect to put somewhere. Happy to extract that out into its own effect, though; it's something that I considered.

useAutoDeploy is only supposed to trigger for restricted users. Perhaps we should rename it useAutoDeployForRestrictedUsers or similar.

I think we're violating SRP here. I would extract it to it's own custom hook.

extensionUpdateRequired,
});

useDeactivateUnassignedDeploymentsEffect(unassignedModComponents);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the fix

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to add to strict null but had to remove due to dependency issues

Base automatically changed from bugfix/7475_fix_switch_deployed_mod to main April 5, 2024 17:56
# Conflicts:
#	src/extensionConsole/pages/deployments/DeploymentsContext.tsx
#	src/extensionConsole/pages/deployments/useAutoDeploy.test.ts
#	src/extensionConsole/pages/deployments/useAutoDeploy.ts
#	src/utils/deploymentUtils.ts
@mnholtz mnholtz enabled auto-merge (squash) April 5, 2024 19:41
@mnholtz mnholtz merged commit b2b87d2 into main Apr 5, 2024
18 of 19 checks passed
@mnholtz mnholtz deleted the bugfix/8131_auto_deactivate_unassigned_mods branch April 5, 2024 19:49
mnholtz added a commit that referenced this pull request Apr 8, 2024
* add test for deactivating unassigned mods

* fix type error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unavailable deployments not uninstalled on Extension Console refresh
3 participants