frontend: PluginSettings: Rework plugin settings local storage usage #2596

vyncent-t · 2024-11-21T18:35:31Z

Description

resolves issue #2595

This PR refactors the way Headlamp handles plugin settings stored in local storage, focusing on simplifying and improving the management of plugin states. Previously, the project stored the entire JSON data of plugin information in headlampPluginSettings, which included unnecessary details. This refactor reduces complexity by saving only the essential state information, improving performance and maintainability.

Key Changes

Simplified Storage:
- Replaced the old headlampPluginSettings structure with enabledPluginsList in local storage.
- The new structure saves only the plugin name and its on/off state, eliminating the need to store complete JSON data.
State Management Updates:
- Introduced the pluginData slice in the plugin state to store plugin information.
- Added enabledPlugins to maintain a list of plugins currently in use.
Backward Compatibility:
- The project now detects the existence of the old headlampPluginSettings in local storage.
- If detected, it converts and migrates the old settings into the new enabledPluginsList format automatically upon visiting the plugin settings page.

Steps to Test

To verify the changes and ensure backward compatibility:

Setup:
- Start on the main branch with multiple plugins installed.
- Disable one plugin while keeping others enabled, and save the settings.
- Open the browser’s local storage and confirm that the disabled plugin’s state is saved in the headlampPluginSettings JSON.
Switch to this Branch:
- Checkout this branch and navigate to the plugin settings page.
Observe Migration:
- Confirm that the page detects the headlampPluginSettings in local storage, migrates the settings into a new enabledPluginsList object, deletes the old headlampPluginSettings, and reloads the page.
- Verify that the plugin states from the previous settings are preserved in the new format.
Post-Migration:
- Ensure that plugin settings updates are correctly saved in the new enabledPluginsList structure.
- Confirm that enabling/disabling plugins reflects accurately on subsequent reloads.

Notes

It improves the performance and clarity of plugin state management by reducing storage overhead.
The migration process ensures a seamless transition for users upgrading from previous versions.

vyncent-t · 2024-11-25T18:15:08Z

last push

allows old storage settings to be migrated to the new settings

vyncent-t · 2024-11-25T18:18:18Z

last push fixes merge conflicts

vyncent-t · 2024-11-27T17:31:01Z

last push rebases and fixes storybook

joaquimrocha

I am a bit surprised this needs so many changes. Wasn't the idea just to remove the saving of the whole package.json from the settings and use just the plugin name instead? Even if we change the name of the key in local storage, or move it to index db, why are we touching the plugin settings this much?

I would also like to understand: are plugins now supposed to be disabled by default? I understood that shipped plugins need to be enabled by default, and that the other ones were also like that for keeping a consistent behavior with the previous versions.

vyncent-t · 2024-12-02T15:42:52Z

noticed some weird behavior when not using any of the old headlampPluginSettings or enabledPluginsList in local storage, working on a fix again

vyncent-t · 2024-12-02T16:43:42Z

some meeting notes

can keep the enabledPluginsList to handle toggles
may need to handle default loading state of a plugin if they come disabled or enabled but most come enabled as of current
handled the backwards compatibility for users on pervious vers that allow migration of old settings to new settings

vyncent-t · 2024-12-06T17:12:43Z

attempting to use the npm link to try to work on the slide plugin changes for updated state

sniok

Left some comments. Let me know if you need any help with testing using npm link

frontend/src/plugin/pluginsSlice.ts

frontend/src/components/App/PluginSettings/PluginSettings.tsx

sniok · 2024-12-09T13:25:08Z

frontend/src/components/App/PluginSettings/PluginSettings.tsx

@@ -284,15 +278,69 @@ export function PluginSettingsPure(props: PluginSettingsPureProps) {
 export default function PluginSettings() {
  const dispatch = useDispatch();

-  const pluginSettings = useTypedSelector(state => state.plugins.pluginSettings);
+  const pluginData = useTypedSelector(state => state.plugins.pluginData);


all this logic should probably be moved to a useEffect

I have tried placing it in useEffect before but it would either error out entirely or cause extra re renders often so I had opted for controlled re renders using the page reload

frontend/src/components/App/PluginSettings/PluginSettings.tsx

Signed-off-by: Vincent T <[email protected]>

joaquimrocha

Left some comments. I think this PR looks too risky to me as it's doing many things when the original reasons why we were touching this part of the code was so we 1. didn't have the plugins version/data coming from a client-side stored version of it; 2. avoid having to store all that data when what we are interested in is checking whether the plugins are enabled on the user's side.

I suggest the following: turn this PR into draft so you keep any relevant changes for later.
And for now let's focus on:

Not having the data for the plugin be populated from the client-side stored version (but instead use whatever comes from the backend)
Keep just the plugin name + isEnabled stored in the headlampPluginSettings record for now. Don't rename it or change the format as I had suggested for now.

Later on we can optimize these changes based on pieces you have here, but I really want to fix the other issues in an isolated way first.

joaquimrocha · 2024-12-10T18:29:23Z

frontend/src/components/App/PluginSettings/PluginSettings.tsx

-  const [pluginChanges, setPluginChanges] = useState(() =>
-    pluginArr.map((plugin: PluginInfo) => {
+  const [pluginChanges, setPluginChanges] = useState<PluginInfo[]>(
+    props.plugins.map((plugin: PluginInfo) => {


Instead of using props. everywhere you need to use the variables, just please declare them as the first line in the function:
const { plugins, ... } = props;
then it's better to use the variables without props.

joaquimrocha · 2024-12-11T11:35:05Z

frontend/src/components/App/PluginSettings/PluginSettings.stories.tsx

+  const enabledList = arr.reduce((acc, p) => {
+    acc[p.name] = !!p.isEnabled;
+    return acc;
+  }, {} as Record<string, boolean>);
+  return enabledList;


This seems overly complex for its purpose. Also it says it creates a list but it creates a record.

joaquimrocha · 2024-12-11T11:41:19Z

frontend/src/components/App/PluginSettings/PluginSettings.tsx

+    dispatch(reloadPage());
+  } else {
+    /**
+     * NOTE: For compatibility with old settings, we need to check if the `localEnabledList` exists.


So we have 2 deprecated items? headlampPluginSettings + enabledPluginsList?
I thought this was just about fixing the former, where we were storing the package.json contents for no reason, when what we were apparently using that list for was just to check if the plugins were enabled.

joaquimrocha · 2024-12-11T11:42:04Z

frontend/src/components/App/PluginSettings/PluginSettings.tsx

-      onSave={plugins => {
-        dispatch(setPluginSettings(plugins));
-        dispatch(reloadPage());
-      }}
+      plugins={pluginData}
+      pluginsEnabledMap={pluginsEnabledMap}
+      onSave={() => {}}


So on save we do nothing? I am confused with what this PR is supposed to fix now.

vyncent-t added the frontend Issues related to the frontend label Nov 21, 2024

vyncent-t self-assigned this Nov 21, 2024

vyncent-t force-pushed the plugin-settings-storage-refactor branch 7 times, most recently from 825bbe1 to 5ba4e15 Compare November 25, 2024 18:10

vyncent-t force-pushed the plugin-settings-storage-refactor branch from 5ba4e15 to 6b0ad82 Compare November 25, 2024 18:17

vyncent-t force-pushed the plugin-settings-storage-refactor branch from 6b0ad82 to fb4cd59 Compare November 27, 2024 17:30

vyncent-t force-pushed the plugin-settings-storage-refactor branch 2 times, most recently from e257f80 to 0e02bf1 Compare November 27, 2024 18:21

vyncent-t changed the title ~~WIP: frontend: PluginSettings: Rework plugin settings local storage usage~~ frontend: PluginSettings: Rework plugin settings local storage usage Nov 27, 2024

vyncent-t marked this pull request as ready for review November 27, 2024 18:32

dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Nov 27, 2024

vyncent-t requested review from sniok and joaquimrocha November 27, 2024 18:33

joaquimrocha reviewed Nov 28, 2024

View reviewed changes

vyncent-t marked this pull request as draft December 2, 2024 15:40

vyncent-t force-pushed the plugin-settings-storage-refactor branch from 0e02bf1 to 5c99964 Compare December 2, 2024 16:38

vyncent-t marked this pull request as ready for review December 2, 2024 16:38

dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Dec 2, 2024

vyncent-t force-pushed the plugin-settings-storage-refactor branch from 5c99964 to 8ff265d Compare December 6, 2024 17:11

vyncent-t force-pushed the plugin-settings-storage-refactor branch 3 times, most recently from 8044116 to a52a8d6 Compare December 6, 2024 17:41

sniok requested changes Dec 9, 2024

View reviewed changes

vyncent-t force-pushed the plugin-settings-storage-refactor branch 2 times, most recently from 260ee6e to 70e720f Compare December 9, 2024 20:16

frontend: PluginSettings: Rework plugin settings local storage usage

99ae0ec

Signed-off-by: Vincent T <[email protected]>

vyncent-t force-pushed the plugin-settings-storage-refactor branch from 70e720f to 99ae0ec Compare December 10, 2024 16:00

joaquimrocha requested changes Dec 11, 2024

View reviewed changes

joaquimrocha marked this pull request as draft December 11, 2024 11:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

frontend: PluginSettings: Rework plugin settings local storage usage #2596

frontend: PluginSettings: Rework plugin settings local storage usage #2596

vyncent-t commented Nov 21, 2024 •

edited

Loading

vyncent-t commented Nov 25, 2024

vyncent-t commented Nov 25, 2024

vyncent-t commented Nov 27, 2024

joaquimrocha left a comment •

edited

Loading

vyncent-t commented Dec 2, 2024

vyncent-t commented Dec 2, 2024

vyncent-t commented Dec 6, 2024

sniok left a comment

sniok Dec 9, 2024

vyncent-t Dec 9, 2024

joaquimrocha left a comment

joaquimrocha Dec 10, 2024

joaquimrocha Dec 11, 2024

joaquimrocha Dec 11, 2024

joaquimrocha Dec 11, 2024

frontend: PluginSettings: Rework plugin settings local storage usage #2596

Are you sure you want to change the base?

frontend: PluginSettings: Rework plugin settings local storage usage #2596

Conversation

vyncent-t commented Nov 21, 2024 • edited Loading

Description

Key Changes

Steps to Test

Notes

vyncent-t commented Nov 25, 2024

vyncent-t commented Nov 25, 2024

vyncent-t commented Nov 27, 2024

joaquimrocha left a comment • edited Loading

Choose a reason for hiding this comment

vyncent-t commented Dec 2, 2024

vyncent-t commented Dec 2, 2024

vyncent-t commented Dec 6, 2024

sniok left a comment

Choose a reason for hiding this comment

sniok Dec 9, 2024

Choose a reason for hiding this comment

vyncent-t Dec 9, 2024

Choose a reason for hiding this comment

joaquimrocha left a comment

Choose a reason for hiding this comment

joaquimrocha Dec 10, 2024

Choose a reason for hiding this comment

joaquimrocha Dec 11, 2024

Choose a reason for hiding this comment

joaquimrocha Dec 11, 2024

Choose a reason for hiding this comment

joaquimrocha Dec 11, 2024

Choose a reason for hiding this comment

vyncent-t commented Nov 21, 2024 •

edited

Loading

joaquimrocha left a comment •

edited

Loading