Mobile: Plugin settings screen: Fix plugin states not set correctly when installing multiple plugins at once (#10580)

Co-authored-by: Laurent Cozic <laurent22@users.noreply.github.com>

packages/app-desktop/gui/ConfigScreen/controls/plugins/PluginsStates.tsx

@@ -1,5 +1,5 @@
 import * as React from 'react';
-import { useCallback, useEffect, useMemo, useState } from 'react';
+import { useCallback, useEffect, useMemo, useRef, useState } from 'react';
 import PluginService, { defaultPluginSetting, Plugins, PluginSetting, PluginSettings } from '@joplin/lib/services/plugins/PluginService';
 import { _ } from '@joplin/lib/locale';
 import styled from 'styled-components';
@@ -214,8 +214,11 @@ export default function(props: Props) {
 		props.onChange({ value: pluginService.serializePluginSettings(event.value) });
 	}, [pluginService, props.onChange]);
 
-	const onDelete = useOnDeleteHandler(pluginSettings, onPluginSettingsChange, false);
+	const pluginSettingsRef = useRef(pluginSettings);
-	const onUpdate = useOnInstallHandler(setUpdatingPluginIds, pluginSettings, repoApi, onPluginSettingsChange, true);
+	pluginSettingsRef.current = pluginSettings;
+
+	const onDelete = useOnDeleteHandler(pluginSettingsRef, onPluginSettingsChange, false);
+	const onUpdate = useOnInstallHandler(setUpdatingPluginIds, pluginSettingsRef, repoApi, onPluginSettingsChange, true);
 
 	const onToolsClick = useCallback(async () => {
 		const template = [

packages/app-desktop/gui/ConfigScreen/controls/plugins/SearchPlugins.tsx

@@ -40,7 +40,10 @@ export default function(props: Props) {
 	const [installingPluginsIds, setInstallingPluginIds] = useState<Record<string, boolean>>({});
 	const [searchResultCount, setSearchResultCount] = useState(null);
 
-	const onInstall = useOnInstallHandler(setInstallingPluginIds, props.pluginSettings, props.repoApi, props.onPluginSettingsChange, false);
+	const pluginSettingsRef = useRef(props.pluginSettings);
+	pluginSettingsRef.current = props.pluginSettings;
+
+	const onInstall = useOnInstallHandler(setInstallingPluginIds, pluginSettingsRef, props.repoApi, props.onPluginSettingsChange, false);
 
 	useEffect(() => {
 		setSearchResultCount(null);

packages/app-mobile/components/screens/ConfigScreen/plugins/utils/usePluginCallbacks.ts

@@ -4,7 +4,7 @@ import useOnInstallHandler from '@joplin/lib/components/shared/config/plugins/us
 import NavService from '@joplin/lib/services/NavService';
 import { PluginSettings, defaultPluginSetting } from '@joplin/lib/services/plugins/PluginService';
 import RepositoryApi from '@joplin/lib/services/plugins/RepositoryApi';
-import { useCallback, useMemo, useState } from 'react';
+import { useCallback, useMemo, useRef, useState } from 'react';
 
 interface Props {
 	updatePluginStates: (settingValue: PluginSettings)=> void;
@@ -44,14 +44,17 @@ const usePluginCallbacks = (props: Props) => {
 		updatePluginEnabled(pluginId, !settings.enabled);
 	}, [props.pluginSettings, updatePluginEnabled]);
 
-	const onDelete = useOnDeleteHandler(props.pluginSettings, onPluginSettingsChange, true);
+	const pluginSettingsRef = useRef(props.pluginSettings);
+	pluginSettingsRef.current = props.pluginSettings;
+
+	const onDelete = useOnDeleteHandler(pluginSettingsRef, onPluginSettingsChange, true);
 
 	const [updatingPluginIds, setUpdatingPluginIds] = useState<Record<string, boolean>>({});
-	const onUpdate = useOnInstallHandler(setUpdatingPluginIds, props.pluginSettings, props.repoApi, onPluginSettingsChange, true);
+	const onUpdate = useOnInstallHandler(setUpdatingPluginIds, pluginSettingsRef, props.repoApi, onPluginSettingsChange, true);
 
 	const [installingPluginIds, setInstallingPluginIds] = useState<Record<string, boolean>>({});
 	const onInstall = useOnInstallHandler(
-		setInstallingPluginIds, props.pluginSettings, props.repoApi, onPluginSettingsChange, false,
+		setInstallingPluginIds, pluginSettingsRef, props.repoApi, onPluginSettingsChange, false,
 	);
 
 	const onShowPluginLog = useCallback((event: ItemEvent) => {

packages/lib/components/shared/config/plugins/useOnDeleteHandler.ts

@@ -1,11 +1,12 @@
 import { _ } from '../../../../locale';
 import PluginService, { PluginSettings, defaultPluginSetting } from '../../../../services/plugins/PluginService';
+import type * as React from 'react';
 import shim from '../../../../shim';
 import produce from 'immer';
 import { ItemEvent, OnPluginSettingChangeHandler } from './types';
 
 const useOnDeleteHandler = (
-	pluginSettings: PluginSettings,
+	pluginSettingsRef: React.RefObject<PluginSettings>,
 	onSettingsChange: OnPluginSettingChangeHandler,
 	deleteNow: boolean,
 ) => {
@@ -15,11 +16,6 @@ const useOnDeleteHandler = (
 		const confirmed = await shim.showConfirmationDialog(_('Delete plugin "%s"?', item.manifest.name));
 		if (!confirmed) return;
 
-		let newSettings = produce(pluginSettings, (draft: PluginSettings) => {
-			if (!draft[item.manifest.id]) draft[item.manifest.id] = defaultPluginSetting();
-			draft[item.manifest.id].deleted = true;
-		});
-
 		if (deleteNow) {
 			const pluginService = PluginService.instance();
 
@@ -29,12 +25,28 @@ const useOnDeleteHandler = (
 			if (plugin) {
 				await pluginService.unloadPlugin(item.manifest.id);
 			}
-
-			newSettings = await pluginService.uninstallPlugins(newSettings);
 		}
 
+		// Important: To avoid race conditions, don't run any async code between fetching plugin
+		// settings from the ref and calling onSettingsChange.
+		let newSettings = pluginSettingsRef.current;
+		if (deleteNow) {
+			newSettings = produce(newSettings, (draft: PluginSettings) => {
+				delete draft[item.manifest.id];
+			});
 			onSettingsChange({ value: newSettings });
-	}, [pluginSettings, onSettingsChange, deleteNow]);
+
+			await PluginService.instance().uninstallPlugin(item.manifest.id);
+		} else {
+			// Setting .deleted causes the app to delete this plugin on startup.
+			newSettings = produce(newSettings, (draft: PluginSettings) => {
+				if (!draft[item.manifest.id]) draft[item.manifest.id] = defaultPluginSetting();
+				draft[item.manifest.id].deleted = true;
+			});
+			onSettingsChange({ value: newSettings });
+		}
+
+	}, [pluginSettingsRef, onSettingsChange, deleteNow]);
 };
 
 export default useOnDeleteHandler;

packages/lib/components/shared/config/plugins/useOnInstallHandler.test.ts

@@ -20,13 +20,13 @@ const itemEvent = ({
 } as ItemEvent);
 const callHook = (isUpdate: boolean, pluginEnabled = true, pluginInstalledViaGUI = true) => () => useOnInstallHandler(
 	setInstallingPluginIds,
-	{
+	{ current: {
 		[pluginId]: pluginInstalledViaGUI ? {
 			enabled: pluginEnabled,
 			deleted: false,
 			hasBeenUpdated: false,
 		} : undefined,
-	},
+	} },
 	repoApi,
 	onPluginSettingsChange,
 	isUpdate,

packages/lib/components/shared/config/plugins/useOnInstallHandler.ts

@@ -13,7 +13,7 @@ type GetRepoApiCallback = ()=> RepositoryApi;
 
 const useOnInstallHandler = (
 	setInstallingPluginIds: React.Dispatch<React.SetStateAction<Record<string, boolean>>>,
-	pluginSettings: PluginSettings,
+	pluginSettingsRef: React.RefObject<PluginSettings>,
 	getRepoApi: GetRepoApiCallback|RepositoryApi,
 	onPluginSettingsChange: OnPluginSettingChangeHandler,
 	isUpdate: boolean,
@@ -45,6 +45,7 @@ const useOnInstallHandler = (
 		}
 
 		if (!installError) {
+			const pluginSettings = pluginSettingsRef.current;
 			const newSettings = produce(pluginSettings, (draft: PluginSettings) => {
 				draft[pluginId] = defaultPluginSetting();
 				if (isUpdate) {
@@ -71,7 +72,7 @@ const useOnInstallHandler = (
 				{ buttons: [_('OK')] },
 			);
 		}
-	}, [getRepoApi, isUpdate, pluginSettings, onPluginSettingsChange, setInstallingPluginIds]);
+	}, [getRepoApi, isUpdate, pluginSettingsRef, onPluginSettingsChange, setInstallingPluginIds]);
 };
 
 export default useOnInstallHandler;