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,66 @@ 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 extensionHandler = jest.fn();
+    const updateTopicSettings = jest.fn((updateSettings) => updateSettings(config));
+    const initPanel = (context: BuiltinPanelExtensionContext) => {
+      context.updatePanelSettingsEditor(
+        { actionHandler: jest.fn(), 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();
+    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,35 @@ 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;
-
-          if (category === "topics" && topicName != undefined) {
-            const topicToSchemaNameMap = getTopicToSchemaNameMap(messagePipelineState());
-            const schemaName = topicToSchemaNameMap[topicName];
-
-            if (schemaName == undefined) {
-              return;
-            }
+      const [category, topicName] = path;
+      if (category !== "topics" || topicName == undefined) {
+        return;
+      }
+      const schemaName = getTopicToSchemaNameMap(messagePipelineState())[topicName];
+      const settings =
+        schemaName != undefined ? extensionsSettings[panelName]?.[schemaName] : undefined;
+      if (!settings) {
+        return;
+      }
 
-            extensionsSettings[panelName]?.[schemaName]?.handler(action, draft.topics[topicName]);
-            setForceConversion((_old) => {
-              return new Set([topicName]);
-            });
-          }
-        }),
-      );
+      updateTopicSettings((draft) => {
+        draft.topics[topicName] ??= {};
+        settings.handler(action, draft.topics[topicName]);
+      });
+      setForceConversion(() => new Set([topicName]));
     };
 
     return {
@@ -643,13 +649,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 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,111 @@ describe("ThreeDeeRender", () => {
     expect(container).toBeInTheDocument();
   });
 
+  it("does not reset nested settings 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: { child: { enabled: false } } } };
+    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") {
+                        (config as { child: { enabled: boolean } }).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 camera movement triggers a later save
+      act(() => {
+        renderer.getCameraState.mockReturnValue({ ...DEFAULT_CAMERA_STATE, distance: 10 });
+        renderer.emit("cameraMove");
+      });
+      act(() => {
+        jest.advanceTimersByTime(100);
+      });
+
+      // THEN the saved nested setting remains enabled
+      expect(savedConfig).toMatchObject({
+        topics: { topic: { child: { enabled: true } } },
+      });
+    } finally {
+      view.unmount();
+      adapterView.unmount();
+      settingsTreeSpy.mockRestore();
+      jest.useRealTimers();
+    }
+  });
+
   it("renders a canvas element", () => {
     // Given
     const props = setup();

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

@@ -358,13 +358,27 @@ export function ThreeDeeRender(props: Readonly<ThreeDeeRenderProps>): React.JSX.
 
   // Rebuild the settings sidebar tree as needed
   useEffect(() => {
-    context.updatePanelSettingsEditor({
-      actionHandler,
-      enableFilter: true,
-      focusedPath: focusedSettingsPath,
-      nodes: settingsTree ?? {},
-    });
-  }, [actionHandler, context, focusedSettingsPath, settingsTree]);
+    context.updatePanelSettingsEditor(
+      {
+        actionHandler,
+        enableFilter: true,
+        focusedPath: focusedSettingsPath,
+        nodes: settingsTree ?? {},
+      },
+      (updateSettings) => {
+        if (!renderer) {
+          return;
+        }
+        // Scene, camera, and layer edits already update the renderer and use the debounced save.
+        // Persist immediately only when a converter handler changes topic settings.
+        const topicsBefore = renderer.config.topics;
+        renderer.updateConfig(updateSettings);
+        if (renderer.config.topics !== topicsBefore) {
+          saveState(renderer.config);
+        }
+      },
+    );
+  }, [actionHandler, context, focusedSettingsPath, settingsTree, renderer, saveState]);
 
   // Update the renderer's reference to `config` when it changes. Note that this does *not*
   // automatically update the settings tree.