fix(panel-extension): prevent nested converter settings from resetting

packages/suite-base/src/components/PanelExtensionAdapter/PanelExtensionAdapter.test.tsx

@@ -22,6 +22,7 @@ import {
   MessageEvent,
   Immutable,
   Subscription,
+  SettingsTreeAction,
 } from "@lichtblick/suite";
 import MockPanelContextProvider from "@lichtblick/suite-base/components/MockPanelContextProvider";
 import { AlertsContext, AlertsContextStore } from "@lichtblick/suite-base/context/AlertsContext";
@@ -71,6 +72,83 @@ describe("PanelExtensionAdapter", () => {
     await sig;
   });
 
+  it("routes nested converter settings through the panel updater", () => {
+    // GIVEN a panel that handles converter settings updates
+    const saveConfig = jest.fn();
+    const updateSettingsTree = jest.fn();
+    const settingsTreeSpy = jest
+      .spyOn(PanelStateContextProvider, "usePanelSettingsTreeUpdate")
+      .mockReturnValue(updateSettingsTree);
+    const config = { topics: { topic: { child: { enabled: false } } } };
+    const panelActionHandler = jest.fn();
+    const extensionHandler = jest.fn();
+    const updateTopicSettings = jest.fn((updateSettings) => updateSettings(config));
+    const initPanel = (context: BuiltinPanelExtensionContext) => {
+      context.updatePanelSettingsEditor(
+        { actionHandler: panelActionHandler, nodes: {} },
+        updateTopicSettings,
+      );
+    };
+
+    const view = render(
+      <MockPanelContextProvider type="3D">
+        <PanelSetup
+          fixture={{
+            topics: [{ name: "topic", schemaName: "test.Schema" }],
+            messageConverters: [
+              {
+                fromSchemaName: "test.Schema",
+                toSchemaName: "foxglove.SceneUpdate",
+                converter: () => ({}),
+                panelSettings: {
+                  "3D": {
+                    settings: () => ({}),
+                    handler: extensionHandler,
+                  },
+                },
+              },
+            ],
+          }}
+        >
+          <PanelExtensionAdapter config={{}} saveConfig={saveConfig} initPanel={initPanel} />
+        </PanelSetup>
+      </MockPanelContextProvider>,
+    );
+
+    // WHEN a nested setting is enabled
+    const action: SettingsTreeAction = {
+      action: "update",
+      payload: { path: ["topics", "topic", "child", "enabled"], input: "boolean", value: true },
+    };
+    act(() => {
+      updateSettingsTree.mock.calls[0]![0].actionHandler(action);
+    });
+
+    // THEN the panel applies the edit once without a duplicate adapter save
+    expect(updateTopicSettings).toHaveBeenCalledTimes(1);
+    expect(extensionHandler).toHaveBeenCalledWith(action, config.topics.topic);
+    expect(extensionHandler).toHaveBeenCalledTimes(1);
+    expect(saveConfig).not.toHaveBeenCalled();
+
+    // WHEN a normal panel setting is changed
+    const panelAction: SettingsTreeAction = {
+      action: "update",
+      payload: { path: ["scene", "enableStats"], input: "boolean", value: true },
+    };
+    act(() => {
+      updateSettingsTree.mock.calls.at(-1)![0].actionHandler(panelAction);
+    });
+
+    // THEN only the panel handler receives the edit without another converter update or save
+    expect(panelActionHandler).toHaveBeenLastCalledWith(panelAction);
+    expect(panelActionHandler).toHaveBeenCalledTimes(2);
+    expect(updateTopicSettings).toHaveBeenCalledTimes(1);
+    expect(extensionHandler).toHaveBeenCalledTimes(1);
+    expect(saveConfig).not.toHaveBeenCalled();
+    view.unmount();
+    settingsTreeSpy.mockRestore();
+  });
+
   it("should expose point-in-time message lookups through the panel context", async () => {
     // GIVEN - a panel that captures its extension context
     let panelContext: PanelExtensionContext | undefined;

packages/suite-base/src/components/PanelExtensionAdapter/PanelExtensionAdapter.tsx

@@ -69,7 +69,11 @@ import { maybeCast } from "@lichtblick/suite-base/util/maybeCast";
 
 import { PanelConfigVersionError } from "./PanelConfigVersionError";
 import { RenderStateConfig, initRenderStateBuilder } from "./renderState";
-import { BuiltinPanelExtensionContext, MessageConverterAlertHandler } from "./types";
+import {
+  BuiltinPanelExtensionContext,
+  MessageConverterAlertHandler,
+  TopicSettingsUpdater,
+} from "./types";
 import { useSharedPanelState } from "./useSharedPanelState";
 import { useSubscribeMessageRange } from "./useSubscribeMessageRange";
 
@@ -392,33 +396,38 @@ function PanelExtensionAdapter(
       },
     };
 
-    const extensionSettingsActionHandler = (action: SettingsTreeAction) => {
+    const extensionSettingsActionHandler = (
+      action: SettingsTreeAction,
+      updateTopicSettings: TopicSettingsUpdater = (updateSettings) => {
+        saveConfig(produce(updateSettings));
+      },
+    ) => {
       if (action.action === "reorder-node") {
         return; // Extensions don't support reordering
       }
       const {
         payload: { path },
       } = action;
 
-      saveConfig(
-        produce<{ topics: Record<string, unknown> }>((draft) => {
-          const [category, topicName] = path;
+      const [category, topicName] = path;
 
-          if (category === "topics" && topicName != undefined) {
-            const topicToSchemaNameMap = getTopicToSchemaNameMap(messagePipelineState());
-            const schemaName = topicToSchemaNameMap[topicName];
+      if (category === "topics" && topicName != undefined) {
+        const topicToSchemaNameMap = getTopicToSchemaNameMap(messagePipelineState());
+        const schemaName = topicToSchemaNameMap[topicName];
 
-            if (schemaName == undefined) {
-              return;
-            }
+        if (schemaName == undefined) {
+          return;
+        }
 
-            extensionsSettings[panelName]?.[schemaName]?.handler(action, draft.topics[topicName]);
-            setForceConversion((_old) => {
-              return new Set([topicName]);
-            });
-          }
-        }),
-      );
+        const settings = extensionsSettings[panelName]?.[schemaName];
+        if (settings) {
+          updateTopicSettings((draft) => {
+            draft.topics[topicName] ??= {};
+            settings.handler(action, draft.topics[topicName]);
+          });
+          setForceConversion(() => new Set([topicName]));
+        }
+      }
     };
 
     return {
@@ -643,13 +652,16 @@ function PanelExtensionAdapter(
         setSubscribedAppSettings(settings);
       },
 
-      updatePanelSettingsEditor: (settings: SettingsTree) => {
+      updatePanelSettingsEditor: (
+        settings: SettingsTree,
+        updateTopicSettings?: TopicSettingsUpdater,
+      ) => {
         if (!isMounted()) {
           return;
         }
         const actionHandler: typeof settings.actionHandler = (action) => {
           settings.actionHandler(action);
-          extensionSettingsActionHandler(action);
+          extensionSettingsActionHandler(action, updateTopicSettings);
         };
         updatePanelSettingsTree({ ...settings, actionHandler });
       },

packages/suite-base/src/components/PanelExtensionAdapter/types.ts

@@ -5,7 +5,12 @@
 // License, v2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at http://mozilla.org/MPL/2.0/
 
-import { Immutable, MessageConverterAlert, PanelExtensionContext } from "@lichtblick/suite";
+import {
+  Immutable,
+  MessageConverterAlert,
+  PanelExtensionContext,
+  SettingsTree,
+} from "@lichtblick/suite";
 import { IteratorResult } from "@lichtblick/suite-base/players/IterablePlayer/IIterableSource";
 import { PlayerAlert, Topic } from "@lichtblick/suite-base/players/types";
 import { InstalledMessageConverter } from "@lichtblick/suite-base/types/messageConverters";
@@ -64,6 +69,12 @@ export type MessagePathDropConfig = {
  * extensions.
  */
 export type BuiltinPanelExtensionContext = {
+  /** Apply converter settings to the panel's current config before saving. */
+  updatePanelSettingsEditor: (
+    settings: SettingsTree,
+    updateTopicSettings?: TopicSettingsUpdater,
+  ) => void;
+
   /**
    * Fetch an asset from Studio's asset manager.
    *
@@ -95,7 +106,11 @@ export type BuiltinPanelExtensionContext = {
    * clears the alert with the given `alertId`.
    */
   unstable_setAlert?: (alertId: string, alert: Immutable<PlayerAlert> | undefined) => void;
-} & PanelExtensionContext;
+} & Omit<PanelExtensionContext, "updatePanelSettingsEditor">;
+
+export type TopicSettingsUpdater = (
+  updateSettings: (config: { topics: Record<string, unknown> }) => void,
+) => void;
 
 export type MessageConverterAlertHandler = (
   converter: InstalledMessageConverter,

packages/suite-base/src/panels/ThreeDeeRender/ThreeDeeRender.test.tsx

@@ -9,18 +9,27 @@
 
 import "@testing-library/jest-dom";
 import { act, render, waitFor } from "@testing-library/react";
+import { produce } from "immer";
 
-import { Topic } from "@lichtblick/suite";
-import { BuiltinPanelExtensionContext } from "@lichtblick/suite-base/components/PanelExtensionAdapter";
+import { SettingsTreeAction, Topic } from "@lichtblick/suite";
+import MockPanelContextProvider from "@lichtblick/suite-base/components/MockPanelContextProvider";
+import {
+  BuiltinPanelExtensionContext,
+  PanelExtensionAdapter,
+} from "@lichtblick/suite-base/components/PanelExtensionAdapter";
 import { useAnalytics } from "@lichtblick/suite-base/context/AnalyticsContext";
 import {
   DEFAULT_FOLLOW_MODE,
   MAX_TRANSFORM_MESSAGES,
 } from "@lichtblick/suite-base/panels/ThreeDeeRender/constants";
 import type { MessageEvent } from "@lichtblick/suite-base/players/types";
+import * as PanelStateContextProvider from "@lichtblick/suite-base/providers/PanelStateContextProvider";
+import PanelSetup from "@lichtblick/suite-base/stories/PanelSetup";
 import MessageEventBuilder from "@lichtblick/suite-base/testing/builders/MessageEventBuilder";
 import RenderStateBuilder from "@lichtblick/suite-base/testing/builders/RenderStateBuilder";
+import { SaveConfig } from "@lichtblick/suite-base/types/panels";
 
+import { RendererConfig } from "./IRenderer";
 import { Renderer } from "./Renderer";
 import { ThreeDeeRender } from "./ThreeDeeRender";
 import { DEFAULT_CAMERA_STATE } from "./camera";
@@ -225,6 +234,139 @@ describe("ThreeDeeRender", () => {
     expect(container).toBeInTheDocument();
   });
 
+  it("saves direct and nested converter edits immediately and preserves them after a later save", () => {
+    // GIVEN a panel with a disabled nested setting
+    jest.useFakeTimers();
+    const renderer = {
+      ...createMockRenderer(),
+      updateConfig: (handler: (draft: RendererConfig) => void) => {
+        renderer.config = produce(renderer.config as RendererConfig, handler);
+        renderer.emit("configChange", renderer);
+      },
+    };
+    mockedRenderer.mockImplementationOnce(() => renderer as unknown as Renderer);
+    const initialConfig = { topics: { topic: { text: "", child: { enabled: false } } } };
+    // The scene handler applies direct fields before the converter handles the same edit.
+    renderer.settings.handleAction.mockImplementation((action: SettingsTreeAction) => {
+      if (action.action === "update" && action.payload.path.length === 3) {
+        renderer.updateConfig((draft) => {
+          (draft.topics.topic as typeof initialConfig.topics.topic).text = action.payload
+            .value as string;
+        });
+      }
+    });
+    let savedConfig: unknown = initialConfig;
+    const saveConfig: SaveConfig<unknown> = (update) => {
+      savedConfig = typeof update === "function" ? update(savedConfig) : update;
+    };
+
+    // AND GIVEN an adapter with a converter that handles nested settings
+    const updateSettingsTree = jest.fn();
+    const settingsTreeSpy = jest
+      .spyOn(PanelStateContextProvider, "usePanelSettingsTreeUpdate")
+      .mockReturnValue(updateSettingsTree);
+    const initPanel = jest.fn<void, [BuiltinPanelExtensionContext]>();
+    const adapterView = render(
+      <MockPanelContextProvider type="3D">
+        <PanelSetup
+          fixture={{
+            topics: [{ name: "topic", schemaName: "test.Schema" }],
+            messageConverters: [
+              {
+                fromSchemaName: "test.Schema",
+                toSchemaName: "foxglove.SceneUpdate",
+                converter: () => ({}),
+                panelSettings: {
+                  "3D": {
+                    settings: () => ({}),
+                    handler: (action, config) => {
+                      if (action.action === "update") {
+                        const topicConfig = config as typeof initialConfig.topics.topic;
+                        if (action.payload.path[2] === "text") {
+                          topicConfig.text = action.payload.value as string;
+                        } else {
+                          topicConfig.child.enabled = action.payload.value as boolean;
+                        }
+                      }
+                    },
+                  },
+                },
+              },
+            ],
+          }}
+        >
+          <PanelExtensionAdapter
+            config={initialConfig}
+            saveConfig={saveConfig}
+            initPanel={initPanel}
+          />
+        </PanelSetup>
+      </MockPanelContextProvider>,
+    );
+
+    // AND GIVEN a 3D panel using the adapter's settings and save methods
+    const context = initPanel.mock.calls.at(-1)![0];
+    const props = setup(undefined, {
+      initialState: context.initialState,
+      saveState: context.saveState.bind(context),
+      updatePanelSettingsEditor: context.updatePanelSettingsEditor.bind(context),
+    });
+    const view = render(<ThreeDeeRender {...props} />);
+
+    try {
+      // WHEN the nested setting is enabled
+      act(() => {
+        updateSettingsTree.mock.calls.at(-1)![0].actionHandler({
+          action: "update",
+          payload: {
+            path: ["topics", "topic", "child", "enabled"],
+            input: "boolean",
+            value: true,
+          },
+        });
+      });
+
+      // THEN the edit is saved
+      expect(savedConfig).toMatchObject({
+        topics: { topic: { child: { enabled: true } } },
+      });
+
+      // WHEN text is typed in a direct field without waiting for the debounced save
+      for (const text of ["h", "he", "hello", "hello world"]) {
+        act(() => {
+          updateSettingsTree.mock.calls.at(-1)![0].actionHandler({
+            action: "update",
+            payload: { path: ["topics", "topic", "text"], input: "string", value: text },
+          });
+        });
+
+        // THEN the settings editor can read the latest text after every edit
+        expect(savedConfig).toMatchObject({
+          topics: { topic: { text, child: { enabled: true } } },
+        });
+      }
+
+      // WHEN camera movement triggers a later save
+      act(() => {
+        renderer.getCameraState.mockReturnValue({ ...DEFAULT_CAMERA_STATE, distance: 10 });
+        renderer.emit("cameraMove");
+      });
+      act(() => {
+        jest.advanceTimersByTime(100);
+      });
+
+      // THEN both converter settings survive the later save
+      expect(savedConfig).toMatchObject({
+        topics: { topic: { text: "hello world", child: { enabled: true } } },
+      });
+    } finally {
+      view.unmount();
+      adapterView.unmount();
+      settingsTreeSpy.mockRestore();
+      jest.useRealTimers();
+    }
+  });
+
   it("renders a canvas element", () => {
     // Given
     const props = setup();