diff --git a/packages/dsh-role-model/client/index.js b/packages/dsh-role-model/client/index.js index c5493ff2..8432a604 100644 --- a/packages/dsh-role-model/client/index.js +++ b/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.`), 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, diff --git a/packages/dsh-role-model/test/client.spec.ts b/packages/dsh-role-model/test/client.spec.ts index 2fcafef0..5de4ea24 100644 --- a/packages/dsh-role-model/test/client.spec.ts +++ b/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; RUNTIME_CHANNELS: readonly { port: number; name: string; runtime: string }[]; + CHANNEL_OPTIONS: readonly { port: number; label: string }[]; settingsRemote: (ctx: unknown) => unknown; readConfig: (ctx: unknown) => Promise | undefined>; buildPatch: ( current: Record, draft: Record, ) => Record; - writeConfig: (ctx: unknown, patch: Record) => Promise; + writeConfig: ( + ctx: unknown, + patch: Record, + timeoutMs?: number, + ) => Promise; } /** 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; 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, revision?: number): Promise => { 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(() => {}); 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.