fix(dsh-role-model): settings panel - declare the Remote inject, representable channels, Apply cannot hang

packages/dsh-role-model/client/index.js

@@ -312,10 +312,14 @@ window.__ModuleLoader__.load({
      */
     const CHANNEL_DEFAULT = 0;
 
-    /** The select's options, the unset state first. */
+    /** The select's options, the unset sentinel first. */
     const CHANNEL_OPTIONS = [
-      { port: CHANNEL_DEFAULT, label: "default — production (:3456)" },
-      ...RUNTIME_CHANNELS.filter((channel) => channel.port !== 3456).map((channel) => ({
+      { port: CHANNEL_DEFAULT, label: "unset — keep the endpoint field (defaults to :3456)" },
+      // Production is listed explicitly rather than folded into the sentinel. `port: 3456`
+      // is the schema default and a legal stored value, so omitting it left the select
+      // carrying a value no option had: the control showed nothing selected and read as
+      // broken. Every channel must be representable AND selectable.
+      ...RUNTIME_CHANNELS.map((channel) => ({
         port: channel.port,
         label: `${channel.name} — ${channel.runtime} (:${channel.port})`,
       })),
@@ -353,18 +357,24 @@ window.__ModuleLoader__.load({
     /**
      * Read the settings Remote, when the context exposes one.
      *
-     * Reached through `ctx.remote` rather than a hard `remote.settings` injection: an
-     * injection edge that cannot resolve means the plugin never mounts, which blanks
-     * the whole settings page — far worse than a form that cannot save. The write path
-     * is guarded the same way, and says so in the UI when the surface is missing.
+     * `remote` and `remote.settings` are declared in `inject` above, which is what makes
+     * them reachable at all — an undeclared namespace throws on property access rather
+     * than answering undefined. The guard stays so that a partially assembled Remote
+     * costs the save button rather than turning every read and write into a thrown error.
      * @param ctx - the client plugin context.
      * @returns the namespace's surface, or undefined when it is not available.
      */
     function settingsRemote(ctx) {
-      const namespace = ctx?.remote?.settings;
-      return namespace !== undefined && typeof namespace.update === "function"
-        ? namespace
-        : undefined;
+      try {
+        const namespace = ctx?.remote?.settings;
+        return namespace !== undefined && typeof namespace.update === "function"
+          ? namespace
+          : undefined;
+      } catch {
+        // A context that cannot answer the read leaves the form usable rather than
+        // turning every read and write into a thrown error.
+        return undefined;
+      }
     }
 
     /**
@@ -438,24 +448,55 @@ window.__ModuleLoader__.load({
       return patch;
     }
 
+    /**
+     * How long to wait for the host to answer a settings write.
+     *
+     * The host serializes every config write with every plugin reload in one unbounded
+     * queue, so a write issued while another profile mutation is in flight (a plugin
+     * install, a reload) is *queued* rather than refused, and its promise may not settle
+     * for minutes. Without a bound the panel sits on "Applying…" forever.
+     */
+    const WRITE_TIMEOUT_MS = 20_000;
+
     /**
      * Write a patch to this plugin's settings namespace.
      * @param ctx - the client plugin context.
      * @param patch - the fields to merge.
+     * @param timeoutMs - how long to wait for an answer before reporting that.
      * @returns undefined on success, or the message to show the user.
      */
-    async function writeConfig(ctx, patch) {
+    async function writeConfig(ctx, patch, timeoutMs = WRITE_TIMEOUT_MS) {
       if (Object.keys(patch).length === 0) return undefined;
       const namespace = settingsRemote(ctx);
       if (namespace === undefined) {
         return "the settings service is unavailable, so this change cannot be saved yet";
       }
+      const timedOut = {};
+      let timer;
+      const deadline = new Promise((resolve) => {
+        timer = setTimeout(
+          () => resolve(timedOut),
+          Number.isFinite(timeoutMs) && timeoutMs > 0 ? timeoutMs : WRITE_TIMEOUT_MS,
+        );
+      });
       try {
-        const response = await namespace.update(SETTINGS_NS, patch, undefined);
+        const write = Promise.resolve(namespace.update(SETTINGS_NS, patch, undefined));
+        // The race may settle on the timeout first; mark a late rejection as handled so it
+        // cannot surface as an unhandled rejection.
+        write.catch(() => undefined);
+        const response = await Promise.race([write, deadline]);
+        if (response === timedOut) {
+          return (
+            "the host has not answered this write yet. Another profile change (a plugin " +
+            "install or reload) may be holding the configuration queue — retry in a moment."
+          );
+        }
         if (response !== undefined && response.ok === true) return undefined;
         return response?.error?.message ?? "the settings service refused the write";
       } catch (error) {
         return error instanceof Error ? error.message : String(error);
+      } finally {
+        clearTimeout(timer);
       }
     }
 
@@ -545,7 +586,17 @@ window.__ModuleLoader__.load({
         setSaving(true);
         setMessage(null);
         const patch = buildPatch(current ?? {}, draft);
-        void writeConfig(context, patch).then((failure) => {
+        // `saving` is cleared outside the happy path too: a write that fails, hangs and
+        // times out, or rejects outright must all return the button to a usable state.
+        // Before this, any outcome other than a resolved-then call left it stuck on
+        // "Applying…" with no message and no way back but a page reload.
+        void (async () => {
+          let failure;
+          try {
+            failure = await writeConfig(context, patch);
+          } catch (error) {
+            failure = error instanceof Error ? error.message : String(error);
+          }
           setSaving(false);
           if (failure === undefined) {
             setCurrent({ ...(current ?? {}), ...patch });
@@ -556,7 +607,7 @@ window.__ModuleLoader__.load({
             return;
           }
           setMessage({ kind: "error", text: failure });
-        });
+        })();
       };
 
       const onReset = () => {
@@ -858,14 +909,17 @@ window.__ModuleLoader__.load({
     }
 
     return {
-      // The settings section slot is provided by the settings shell, so wait for it.
+      // The settings section slot is provided by the settings shell, and the Remote
+      // namespaces this page reads must be declared here.
       //
-      // `remote.settings` is deliberately NOT injected. A namespace is a Cordis child
-      // service (`remote.<namespace>`), and an injection edge that cannot resolve keeps
-      // the plugin from mounting at all — which blanks the entire settings page. The
-      // Remote is instead read through `ctx.remote` behind a guard, so an unavailable
-      // settings surface costs the save button, not the page.
-      inject: ["slots"],
+      // A Remote namespace is assembled for the entries that declare it: without the
+      // entry, `ctx.remote` **throws** `cannot get property "remote" without inject` and
+      // `ctx.get("remote")` cannot find it either, so the service is simply not reachable.
+      // Declaring only `slots` produced both live symptoms — the raw Cordis error on Apply,
+      // and a form whose every field stayed empty because the read failed silently. Every
+      // client plugin that touches the Remote lists it, including the settings mirror
+      // (`ui-settings`: `['remote', 'remote.settings']`).
+      inject: ["slots", "remote", "remote.settings"],
 
       apply(ctx) {
         panelContext = ctx;
@@ -901,6 +955,7 @@ window.__ModuleLoader__.load({
         CONFIG_FIELDS,
         SELECT_FIELDS,
         RUNTIME_CHANNELS,
+        CHANNEL_OPTIONS,
         settingsRemote,
         readConfig,
         buildPatch,

packages/dsh-role-model/test/client.spec.ts

@@ -576,17 +576,24 @@ describe("editing configuration from the panel", () => {
     CONFIG_FIELDS: readonly string[];
     SELECT_FIELDS: ReadonlySet<string>;
     RUNTIME_CHANNELS: readonly { port: number; name: string; runtime: string }[];
+    CHANNEL_OPTIONS: readonly { port: number; label: string }[];
     settingsRemote: (ctx: unknown) => unknown;
     readConfig: (ctx: unknown) => Promise<Record<string, unknown> | undefined>;
     buildPatch: (
       current: Record<string, unknown>,
       draft: Record<string, unknown>,
     ) => Record<string, unknown>;
-    writeConfig: (ctx: unknown, patch: Record<string, unknown>) => Promise<string | undefined>;
+    writeConfig: (
+      ctx: unknown,
+      patch: Record<string, unknown>,
+      timeoutMs?: number,
+    ) => Promise<string | undefined>;
   }
 
   /** A `ctx.remote.settings` double recording writes. */
-  function fakeSettings(options: { updateError?: string; describeError?: string } = {}) {
+  function fakeSettings(
+    options: { updateError?: string; describeError?: string; updateHangs?: boolean } = {},
+  ) {
     const writes: { ns: string; patch: Record<string, unknown>; revision?: number }[] = [];
     const current = {
       endpoint: "http://127.0.0.1:3456",
@@ -611,6 +618,9 @@ describe("editing configuration from the panel", () => {
         ),
       update: (ns: string, patch: Record<string, unknown>, revision?: number): Promise<Answer> => {
         writes.push({ ns, patch, ...(revision === undefined ? {} : { revision }) });
+        // Models a write the host accepts but never answers, which is what happens while
+        // another profile mutation holds the loader's exclusive queue.
+        if (options.updateHangs === true) return new Promise<Answer>(() => {});
         return Promise.resolve(
           options.updateError === undefined
             ? { ok: true, value: {} }
@@ -637,12 +647,17 @@ describe("editing configuration from the panel", () => {
     new Function("window", `${readFileSync(clientEntry, "utf8")}\n`)(stub);
     if (registered === undefined) throw new Error("the client entry registered no module");
     const components: ((props: unknown) => unknown)[] = [];
+    const remoteService = { settings };
     const ctx = {
       effect: (callback: () => unknown) => {
         callback();
         return () => undefined;
       },
-      remote: { settings },
+      // A Remote namespace is gated by `inject`: the boot assembles it for the entries
+      // that declare it, which is why every client plugin that touches `ctx.remote` lists
+      // it. This double therefore mirrors the post-inject context — the property resolves —
+      // and the declaration itself is asserted separately.
+      remote: remoteService,
       slots: {
         inject: (_key: string, callback: () => void) => callback(),
         register: (_options: unknown, panel: unknown) => {
@@ -668,13 +683,9 @@ describe("editing configuration from the panel", () => {
     return { component, inject: plugin.inject ?? [], internals: plugin.__internals, ctx };
   }
 
-  test("mounts on the settings slot alone, so an absent Remote cannot blank the page", () => {
+  test("mounts on the settings slot, which the settings shell provides", () => {
     const { inject } = applyWith(fakeSettings().settings);
     expect(inject).toContain("slots");
-    // A Remote namespace is a Cordis child service, and an injection edge that cannot
-    // resolve stops the plugin mounting — which blanks the whole settings page. The
-    // Remote is read through `ctx.remote` behind a guard instead.
-    expect(inject).not.toContain("remote.settings");
   });
 
   /**
@@ -746,15 +757,31 @@ describe("editing configuration from the panel", () => {
     for (const child of panelChildren()) walk(child);
     expect(select, "no port select").toBeDefined();
     const options = childrenOf(select as ExpandedElement).filter(isElement);
-    // The unset state comes first and names the default it resolves to; production is
-    // not listed twice, because choosing it is the same as leaving the field alone.
+    // The unset sentinel comes first, then every channel as an explicit, selectable port.
     expect(options[0]?.props.value).toBe(0);
-    expect(childrenOf(options[0] as ExpandedElement).join("")).toContain("default");
-    expect(options[0] && childrenOf(options[0]).join("")).toContain("3456");
-    expect(options.map((option) => option.props.value)).toEqual([0, 3457, 3458]);
+    expect(options.map((option) => option.props.value)).toEqual([0, 3456, 3457, 3458]);
     const labels = options.map((option) => childrenOf(option).join(""));
-    expect(labels[1]).toContain("stage");
-    expect(labels[2]).toContain("development");
+    expect(labels[1]).toContain("production");
+    expect(labels[2]).toContain("stage");
+    expect(labels[3]).toContain("development");
+  });
+
+  /**
+   * Every runtime channel must be selectable, including production 3456.
+   *
+   * `port: 3456` is the schema default and a legal stored value. When the option list
+   * omits it the select carries a value no option has, so the channel control shows
+   * nothing selected and reads as broken — the reported "the UI that chooses
+   * dev/stage/prod doesn't work".
+   */
+  test("offers every runtime channel as a selectable option", () => {
+    const { internals } = applyWith(fakeSettings().settings);
+    const offered = internals.CHANNEL_OPTIONS.map((option) => option.port);
+    for (const channel of internals.RUNTIME_CHANNELS) {
+      expect(offered, `channel ${String(channel.port)} is not selectable`).toContain(channel.port);
+    }
+    // The unset sentinel stays distinct from a deliberate choice of production.
+    expect(offered).toContain(0);
   });
 
   test("the port is a closed choice, not free text", () => {
@@ -813,6 +840,55 @@ describe("editing configuration from the panel", () => {
     expect(writes).toEqual([]);
   });
 
+  /**
+   * A write the host accepts but never answers must not hang the panel forever.
+   *
+   * The settings write runs inside the loader's exclusive queue, which is an unbounded
+   * FIFO: while another profile mutation holds it (a plugin install, a reload), the write
+   * is queued and the Remote call simply never resolves. The panel then sat on
+   * "Applying…" indefinitely with no message and no way back but a page reload.
+   */
+  test("stops waiting when the host never answers a write", async () => {
+    const { settings, writes } = fakeSettings({ updateHangs: true });
+    const { internals, ctx } = applyWith(settings);
+    const failure = await internals.writeConfig(ctx, { port: 3458 }, 20);
+    // The write was attempted, and the panel gets a message it can show.
+    expect(writes.length).toBe(1);
+    expect(typeof failure).toBe("string");
+    expect(failure).toContain("has not answered");
+  }, 1500);
+
+  /**
+   * The Remote namespaces must be declared in `inject`.
+   *
+   * A Remote namespace is assembled for the entries that declare it, so an undeclared
+   * `ctx.remote` THROWS (`cannot get property "remote" without inject`) and `ctx.get`
+   * cannot find it either — the service is not there to look up. Declaring only `slots`
+   * is what produced both live symptoms: the raw Cordis error on Apply, and a form whose
+   * every field rendered empty because the read failed silently.
+   *
+   * Every client plugin that touches `ctx.remote` lists it, including the settings mirror
+   * (`ui-settings`: `['remote', 'remote.settings']`).
+   */
+  test("declares the Remote namespaces it reads in inject", () => {
+    const { inject } = applyWith(fakeSettings().settings);
+    expect(inject).toContain("slots");
+    expect(inject, "ctx.remote would throw without this").toContain("remote");
+    expect(inject, "ctx.remote.settings would be absent without this").toContain("remote.settings");
+  });
+
+  test("a context whose Remote cannot be read degrades instead of throwing", async () => {
+    const { internals } = applyWith(fakeSettings().settings);
+    const hostile = {
+      get remote(): unknown {
+        throw new Error('cannot get property "remote" without inject');
+      },
+    };
+    // A guard that cannot read the Remote must leave the form usable, not throw.
+    expect(internals.settingsRemote(hostile)).toBeUndefined();
+    await expect(internals.writeConfig(hostile, { port: 3458 })).resolves.toContain("unavailable");
+  });
+
   test("a context without the settings Remote degrades instead of throwing", async () => {
     // This is the case that matters: the page must still render when the Remote is
     // absent, so every access is guarded rather than assumed.