mirror of
https://github.com/MervinPraison/PraisonAI.git
synced 2026-11-07 04:35:58 -10:00
fix(mobile): Settings offers Remove only when there is a key, and refuses a bad address (#4847)
* fix(mobile): Settings offers Remove only when there is a key, and refuses a bad address Four defects, all on the Settings screen, all seen on a device running main. 1. REMOVE WAS OFFERED WITH NOTHING TO REMOVE. The OpenAI key row read "Not set" and a `Remove` button sat under it -- a control that destroys nothing and reports nothing, which is the defect class this package spent two days eliminating everywhere else, surviving in the screen that introduced it. `canClear` is now computed in the view model (`state === "configured"`, not `!== "not-set"`: an UNRESOLVED keychain lookup is not a key), and the presence refresh flips the button in the SAME pass that repaints the word, so the two cannot disagree. 2. THE ENGINE ADDRESS RENDERED AS IF IT APPLIED WHEN IT COULD NOT. Its own help said "Only used by the remote engine" while it accepted edits under the in-process engine, which runs on the device and never contacts an address. `SettingDef.appliesWhen` lets the REGISTRY say which setting switches another on; the field is now disabled with "Set Engine to remote-http to use this." Disabled rather than hidden, and it is a close call. A field that disappears takes the typed value off the screen with it -- the store keeps it, but the user cannot see that it is kept, and someone flipping between engines watches their address vanish and come back. And an address is what a user looks for BEFORE choosing the engine that uses it: a row that is not there cannot teach that the two are connected. A test pins the round trip: type an address, switch to in-process, switch back, and it is still there. 3. A MALFORMED ADDRESS WAS ACCEPTED IN SILENCE. `7.0.0.1:8765` was stored, displayed back, and discovered to be wrong only when a turn failed. The refusal PATH has existed since #4694 (validateInput -> set -> a `role="alert"` note beside the field); what was missing is that `baseUrl` had no `validate` at all, so nothing was ever handed to it to refuse. `httpUrl()` in core/settings/store.ts is that half. It refuses everything `new URL` will not parse AND every scheme that is not http(s) -- so `localhost:8765`, which parses as a URL whose protocol is `localhost:`, does not slip through -- and it normalises what it accepts, so one engine is not three settings that differ by a trailing slash. The message names an address that WOULD work, because naming only the setting leaves the user looking at the same string with no idea which part was wrong. 4. THE CREDENTIAL WAS FILED UNDER "ENGINE". It is not an engine setting, it is the one field that decides whether the app answers at all, and it sat third of three at the same weight as a picker and an address. It now leads the screen in a section of its own. Registry order IS screen order, so the move is the whole of the reordering; `data-lead` marks the first section so the stylesheet can give it weight without the renderer knowing which one it is. app.css styled `.section-heading` while the app paints `.settings-section`, so every heading had been rendering at the browser's default h3 -- fixed, on the class the app actually emits. Also: every def has carried `help` since the registry was written and NOTHING rendered it. The screen painted a label, a control and a refusal note and dropped the sentence explaining what the field is for -- the same defect as an inert setting, one layer along. It is on screen now. Gates: each of the four defects, reintroduced, kills a named test -- offering Remove with no key, disabling the address under the remote engine, dropping the validator, and moving the key back under "Engine". Both the first PAINT and the later sync walk are covered; a renderer corrected only by the sync passed every end-to-end test while painting the defect. Verified on an Android 15 emulator, light and dark: no key (no Remove), key set (Remove offered and working), in-process (address off, with the sentence that turns it on), remote (address live), `7.0.0.1:8765` refused on screen, `http://10.0.0.7:9000/` accepted and normalised. Co-Authored-By: Claude Opus 5 <[email protected]> * fix(mobile): reject query/fragment engine addresses and associate settings help via aria-describedby httpUrl() preserved url.search, but the remote-http engine builds `${base}/chat` by concatenation, so a stored "http://host:8765/?token=abc" became "http://host:8765/?token=abc/chat" -- a request for "/" with a mangled query that reaches no endpoint, while Settings reported a successful save. Reject any address carrying a query or fragment at the point of acceptance. Help and inactive notes were visual siblings of their controls but not programmatically associated, so a screen-reader user heard only the field's aria-label. Give the notes stable ids and point the control's aria-describedby at them, keeping the inactive id in step with the disabled state in syncSettings. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Mervin Praison <[email protected]> --------- Co-authored-by: Claude Opus 5 <[email protected]> Co-authored-by: praisonai-triage-agent[bot] <272766704+praisonai-triage-agent[bot]@users.noreply.github.com> Co-authored-by: Mervin Praison <[email protected]>
This commit is contained in:
12 files changed
+1209
-54
No files matched your search
@@ -388,6 +388,22 @@ button:disabled { opacity: .45; cursor: default; }
|
||||
border: 1px solid var(--rule); border-radius: 8px; background: var(--panel); color: inherit; }
|
||||
.setting-error { display: block; margin: .1rem 0 0; color: var(--warn); font-size: .84rem; }
|
||||
.setting-error[hidden] { display: none; }
|
||||
/* The `help` every def has carried since the registry was written, painted for
|
||||
the first time. It sits between the label and the control, so it is read
|
||||
before the field rather than as a footnote under it. */
|
||||
.row-setting .setting-help { display: block; margin: 0; color: var(--soft); font-size: .84rem; }
|
||||
/* "Set Engine to remote-http to use this." Not `--warn`: nothing is wrong, the
|
||||
field is simply switched off, and a warning colour on a normal state is how
|
||||
a screen teaches people to ignore warnings. `[hidden]` explicitly, for the
|
||||
same reason `.setting-error[hidden]` is: this rule's `display: block` is an
|
||||
author declaration and beats the user agent's `[hidden] { display: none }`. */
|
||||
.row-setting .setting-inactive { display: block; margin: .1rem 0 0; color: var(--soft); font-size: .84rem; }
|
||||
.row-setting .setting-inactive[hidden] { display: none; }
|
||||
/* A field nothing reads, drawn as a field nothing reads. `opacity` alone would
|
||||
also fade the label and the help, which are still worth reading -- this is on
|
||||
the control only. */
|
||||
.row-setting .setting-input:disabled,
|
||||
.row-setting select:disabled { color: var(--soft); background: transparent; border-style: dashed; }
|
||||
|
||||
/* A secret row. The presence word sits above the field, so "Configured" is the
|
||||
first thing read -- the answer to the only question a masked field cannot
|
||||
@@ -397,6 +413,31 @@ button:disabled { opacity: .45; cursor: default; }
|
||||
.setting-clear { font: inherit; font-size: .84rem; align-self: flex-start;
|
||||
margin-top: .25rem; padding: .3rem .6rem; color: var(--ink);
|
||||
background: none; border: 1px solid var(--rule); border-radius: .4rem; }
|
||||
/* Stated explicitly, exactly like `.setting-error[hidden]`: `align-self` does
|
||||
not create a display declaration but `.setting-clear` is a flex ITEM, and the
|
||||
button is hidden by main.ts the moment there is no key to remove. An
|
||||
always-visible Remove under a row reading "Not set" is the defect this rule
|
||||
is half of. */
|
||||
.setting-clear[hidden] { display: none; }
|
||||
|
||||
/*
|
||||
* The settings screen's information architecture.
|
||||
*
|
||||
* `buildSettingsScreen` has emitted `<h3 class="settings-section">` since it
|
||||
* was written and this stylesheet styled `.section-heading`, which nothing
|
||||
* paints -- so every heading rendered at the browser's default `h3`, and the
|
||||
* screen read as one flat list in which a credential and an engine address had
|
||||
* the same weight. The lead section is the one a new user came for: it keeps
|
||||
* the ink colour while the rest go soft, and its rows sit in a panel so the
|
||||
* eye lands there first.
|
||||
*/
|
||||
.screen-settings .settings-section { font-size: .72rem; letter-spacing: .1em;
|
||||
text-transform: uppercase; color: var(--soft); margin: 1.4rem 0 .3rem; font-weight: 600; }
|
||||
.screen-settings .settings-section:first-of-type { margin-top: .6rem; }
|
||||
.screen-settings .settings-section[data-lead] { color: var(--ink); }
|
||||
.screen-settings .row-setting .setting-label { font-weight: 550; }
|
||||
.screen-settings .row-setting[data-lead] { background: var(--panel); border-radius: var(--radius);
|
||||
padding: .6rem .75rem; }
|
||||
|
||||
.section-heading { font-size: .72rem; letter-spacing: .1em; text-transform: uppercase;
|
||||
color: var(--soft); margin: 1.2rem 0 .3rem; }
|
||||
|
||||
@@ -224,3 +224,90 @@ test("the empty state keeps clear of the safe area", () => {
|
||||
}
|
||||
assert.match(value, /\+\s*[\d.]+rem/, `the gutter must survive a zero inset: ${value}`);
|
||||
});
|
||||
|
||||
/**
|
||||
* Rules parsed by BRACE MATCHING, not by a `}`-anchored regex.
|
||||
*
|
||||
* `[^}]*` stops at the first closing brace it meets, which for a declaration
|
||||
* containing `calc(...)` or a nested block is not the end of the rule -- an
|
||||
* earlier agent's colour test silently matched nothing and passed over a rule
|
||||
* it had never seen. This walks the stylesheet, so a selector that is absent is
|
||||
* absent rather than "not matched".
|
||||
*/
|
||||
function rules(source: string): readonly { selector: string; body: string }[] {
|
||||
const found: { selector: string; body: string }[] = [];
|
||||
let index = 0;
|
||||
while (index < source.length) {
|
||||
const open = source.indexOf("{", index);
|
||||
if (open === -1) break;
|
||||
let depth = 1;
|
||||
let cursor = open + 1;
|
||||
while (cursor < source.length && depth > 0) {
|
||||
if (source[cursor] === "{") depth += 1;
|
||||
else if (source[cursor] === "}") depth -= 1;
|
||||
cursor += 1;
|
||||
}
|
||||
const selector = source.slice(index, open).trim();
|
||||
const body = source.slice(open + 1, cursor - 1);
|
||||
// An at-rule (`@media`) holds rules rather than declarations; recurse into
|
||||
// it so a token redefined for dark mode is found under its own selector.
|
||||
if (selector.startsWith("@")) found.push(...rules(body));
|
||||
else found.push({ selector, body });
|
||||
index = cursor;
|
||||
}
|
||||
return found;
|
||||
}
|
||||
|
||||
/** Every rule whose selector list contains `wanted`, asserted non-empty -- so a
|
||||
* selector that was renamed fails here instead of passing vacuously. */
|
||||
function rulesFor(wanted: string): readonly { selector: string; body: string }[] {
|
||||
const matched = rules(css).filter((rule) =>
|
||||
rule.selector.split(",").some((part) => part.trim() === wanted),
|
||||
);
|
||||
assert.ok(matched.length > 0, `no rule in app.css has the selector "${wanted}"`);
|
||||
return matched;
|
||||
}
|
||||
|
||||
test("a hidden Remove button is actually hidden", () => {
|
||||
// `.setting-clear` is a flex ITEM in a `.row-setting` flex column, and
|
||||
// main.ts sets `hidden` on it the moment there is no key to remove. The
|
||||
// pairing this file already asserts for `.screen[hidden]` applies: an author
|
||||
// `display` declaration beats the user agent's `[hidden] { display: none }`,
|
||||
// and the failure mode is the exact defect being fixed -- a `Remove` button
|
||||
// under a row that reads "Not set".
|
||||
const hiding = rulesFor(".setting-clear[hidden]");
|
||||
assert.ok(
|
||||
hiding.some((rule) => /display\s*:\s*none/.test(rule.body)),
|
||||
`.setting-clear[hidden] must set display: none -- found: ${hiding.map((r) => r.body).join(" | ")}`,
|
||||
);
|
||||
});
|
||||
|
||||
test("a field that is switched off says so in the note, not in a warning colour", () => {
|
||||
// "Set Engine to remote-http to use this" is not an error. Painting it
|
||||
// `--warn` beside a field the user has not touched teaches people that the
|
||||
// warning colour means nothing, which is what it costs when a real refusal
|
||||
// appears in `.setting-error` two lines below.
|
||||
const inactive = rulesFor(".row-setting .setting-inactive");
|
||||
const body = inactive.map((r) => r.body).join(" ");
|
||||
assert.match(body, /color\s*:\s*var\(--soft\)/, `the inactive note must be soft, not loud: ${body}`);
|
||||
assert.equal(/var\(--warn\)/.test(body), false, `nothing is wrong: ${body}`);
|
||||
// And it hides properly, for the same author-declaration reason as above.
|
||||
const hidden = rulesFor(".row-setting .setting-inactive[hidden]");
|
||||
assert.ok(hidden.some((rule) => /display\s*:\s*none/.test(rule.body)), "an empty note must not paint");
|
||||
});
|
||||
|
||||
test("the settings sections are styled on the class the app actually emits", () => {
|
||||
// app.css styled `.section-heading`; `buildSettingsScreen` emits
|
||||
// `.settings-section`. So every heading rendered at the browser's default
|
||||
// `h3` and the screen read as one flat list -- a credential and an engine
|
||||
// address at the same weight. The class names have to be the SAME name.
|
||||
assert.ok(main.includes('className = "settings-section"'), "main.ts still emits settings-section");
|
||||
const styled = rulesFor(".screen-settings .settings-section");
|
||||
const body = styled.map((r) => r.body).join(" ");
|
||||
assert.match(body, /text-transform|font-size|letter-spacing/, `a heading must look like one: ${body}`);
|
||||
// The lead section keeps the ink colour while the others go soft: that is the
|
||||
// hierarchy, and it is the whole reason main.ts writes `data-lead`.
|
||||
assert.ok(main.includes('dataset["lead"]'), "main.ts must mark the lead section");
|
||||
const lead = rulesFor('.screen-settings .settings-section[data-lead]');
|
||||
assert.match(lead.map((r) => r.body).join(" "), /color\s*:\s*var\(--ink\)/);
|
||||
});
|
||||
@@ -56,7 +56,7 @@ import { createFakeStorage } from "../../testing/src/fake-storage.ts";
|
||||
import { createFakeSecrets } from "../../testing/src/fake-secrets.ts";
|
||||
import { createFakeHttp, sseResponse, streamOf } from "../../testing/src/fake-http.ts";
|
||||
import { PROTOCOL_VERSION } from "../../protocol/src/version.ts";
|
||||
import { appEngines, mount, EMPTY_TITLE_ID } from "./main.ts";
|
||||
import { appEngines, buildSettingsScreen, mount, EMPTY_TITLE_ID } from "./main.ts";
|
||||
import { ENGINE_PRAISONAI_TS, ENGINE_REMOTE_HTTP, SETTING_DEFS } from "./registry.ts";
|
||||
import { createSettingsStore, facadeFor, type SettingsFacade } from "../../core/src/settings/store.ts";
|
||||
import type { Platform } from "./platform.ts";
|
||||
@@ -3031,3 +3031,319 @@ test("the chrome is never a blank rectangle, not even while boot is still runnin
|
||||
assert.ok(emptyWords(dom).includes(en.emptyNeedsKeyTitle), emptyWords(dom));
|
||||
await app?.dispose();
|
||||
});
|
||||
|
||||
// ---- the settings screen's four defects, on the real composition root -------
|
||||
//
|
||||
// All four were seen on a device running main: a `Remove` under a key that read
|
||||
// "Not set", an engine address that rendered as if it applied while the
|
||||
// in-process engine answered, a malformed address accepted in silence, and the
|
||||
// credential filed under a heading called "Engine". Everything above asserts a
|
||||
// piece in isolation; these drive `mount()` and read the DOM the user taps.
|
||||
|
||||
/** The Remove button for one secret row, whether or not it is showing. */
|
||||
const removeButton = (dom: ReturnType<typeof createFakeDom>, key: string) =>
|
||||
dom.find((n) => n.dataset["action"] === "clear-secret" && n.dataset["settingKey"] === key);
|
||||
|
||||
|
||||
test("Remove is NOT offered while the key row says Not set", async () => {
|
||||
// The defect. A control that cannot do anything, sitting under the word that
|
||||
// says there is nothing for it to do.
|
||||
const { dom, platform } = harness();
|
||||
const app = await mount({ root: dom.root as never, platform, now: () => 1, newChatId: () => "c1" });
|
||||
await openSettings(dom);
|
||||
|
||||
assert.equal(presenceOf(dom, "openaiApiKey"), "Not set");
|
||||
const remove = removeButton(dom, "openaiApiKey");
|
||||
assert.ok(remove !== null, "the row is built with the control, it is simply not offered");
|
||||
assert.equal(remove.hidden, true, `Remove must not be offered with no key:\n${dom.text()}`);
|
||||
// Hidden AND disabled: the click handler is delegated on root, so a hidden
|
||||
// node still answers a click that reaches it.
|
||||
assert.equal(remove.disabled, true);
|
||||
await app?.dispose();
|
||||
});
|
||||
|
||||
test("Remove appears the moment a key is stored, and it works -- the pair", async () => {
|
||||
// Without this, hiding it unconditionally would satisfy the test above and
|
||||
// "no key" would become a state nobody can get back OUT of.
|
||||
const { dom, platform, secrets } = harness();
|
||||
const app = await mount({ root: dom.root as never, platform, now: () => 1, newChatId: () => "c1" });
|
||||
await openSettings(dom);
|
||||
assert.equal(removeButton(dom, "openaiApiKey")?.hidden, true);
|
||||
|
||||
const field = keyField(dom);
|
||||
field!.value = "sk-fresh";
|
||||
dom.change(field as never);
|
||||
await settle();
|
||||
|
||||
const remove = removeButton(dom, "openaiApiKey");
|
||||
assert.equal(remove?.hidden, false, "a stored key must be removable");
|
||||
assert.equal(remove?.disabled, false);
|
||||
dom.click(remove as never);
|
||||
await settle();
|
||||
|
||||
assert.equal(await secrets.has({ slot: "openai", account: "default" }), false);
|
||||
assert.equal(presenceOf(dom, "openaiApiKey"), "Not set");
|
||||
// And the button goes away again in the SAME pass that repaints the word.
|
||||
assert.equal(removeButton(dom, "openaiApiKey")?.hidden, true, "the offer must go with the key");
|
||||
await app?.dispose();
|
||||
});
|
||||
|
||||
test("the engine address is switched OFF while the in-process engine answers", async () => {
|
||||
// Its own help text said "Only used by the remote engine" and it rendered
|
||||
// identically while `praisonai-ts` -- which runs on the device and never
|
||||
// contacts an address -- was selected.
|
||||
const { dom, platform } = harness();
|
||||
const app = await mount({ root: dom.root as never, platform, now: () => 1, newChatId: () => "c1" });
|
||||
await openSettings(dom);
|
||||
|
||||
const engine = settingField(dom, "Engine");
|
||||
assert.ok(engine, `there must be an engine picker:\n${dom.text()}`);
|
||||
engine.value = ENGINE_PRAISONAI_TS;
|
||||
dom.change(engine as never);
|
||||
await settle();
|
||||
|
||||
const address = settingField(dom, "Engine address");
|
||||
// SHOWN, not hidden. A field that disappears takes the value the user typed
|
||||
// off the screen with it, and cannot teach anyone that the two settings are
|
||||
// connected before they have chosen the engine that uses it.
|
||||
assert.ok(address, `the address must still be on screen:\n${dom.text()}`);
|
||||
assert.equal(address.disabled, true, "nothing reads it under this engine");
|
||||
const note = dom.find((n) => n.dataset["settingInactive"] === "baseUrl");
|
||||
assert.ok(note !== null, "a field that is off must say what turns it on");
|
||||
assert.equal(note.hidden, false);
|
||||
assert.equal(note.textContent, en.settingInactive("Engine", ENGINE_REMOTE_HTTP));
|
||||
await app?.dispose();
|
||||
});
|
||||
|
||||
test("the engine address is LIVE under the remote engine -- the pair", async () => {
|
||||
// The recovery path a phone depends on (#4694): reach no 127.0.0.1:8765,
|
||||
// open Settings, type the engine's real address. Disabling it always would
|
||||
// close that path for good.
|
||||
const { dom, platform } = harness();
|
||||
const app = await mount({ root: dom.root as never, platform, now: () => 1, newChatId: () => "c1" });
|
||||
await openSettings(dom);
|
||||
|
||||
const address = settingField(dom, "Engine address");
|
||||
assert.equal(address?.disabled, false, "the web starts on the remote engine");
|
||||
assert.equal(dom.find((n) => n.dataset["settingInactive"] === "baseUrl")?.hidden, true);
|
||||
await app?.dispose();
|
||||
});
|
||||
|
||||
test("switching back to the remote engine gives the address back, with what was typed", async () => {
|
||||
// The reason it is disabled rather than hidden: a value the user typed must
|
||||
// survive a round trip through the other engine, and be VISIBLE surviving it.
|
||||
const { dom, platform } = harness();
|
||||
const app = await mount({ root: dom.root as never, platform, now: () => 1, newChatId: () => "c1" });
|
||||
await openSettings(dom);
|
||||
|
||||
const address = settingField(dom, "Engine address");
|
||||
address!.value = "http://10.0.0.7:9000";
|
||||
dom.change(address as never);
|
||||
await settle();
|
||||
|
||||
const engine = settingField(dom, "Engine");
|
||||
engine!.value = ENGINE_PRAISONAI_TS;
|
||||
dom.change(engine as never);
|
||||
await settle();
|
||||
assert.equal(settingField(dom, "Engine address")?.value, "http://10.0.0.7:9000", "still shown");
|
||||
assert.equal(settingField(dom, "Engine address")?.disabled, true);
|
||||
|
||||
engine!.value = ENGINE_REMOTE_HTTP;
|
||||
dom.change(engine as never);
|
||||
await settle();
|
||||
const back = settingField(dom, "Engine address");
|
||||
assert.equal(back?.disabled, false);
|
||||
assert.equal(back?.value, "http://10.0.0.7:9000", "the typed address must survive the round trip");
|
||||
await app?.dispose();
|
||||
});
|
||||
|
||||
test("a malformed engine address is REFUSED on screen, with an address that would work", async () => {
|
||||
// `7.0.0.1:8765` was stored, displayed back, and discovered to be wrong only
|
||||
// when a turn failed. There is no Save button -- a field commits on blur --
|
||||
// so the refusal has to be the thing that tells the user, immediately.
|
||||
const { dom, platform } = harness();
|
||||
const app = await mount({ root: dom.root as never, platform, now: () => 1, newChatId: () => "c1" });
|
||||
await openSettings(dom);
|
||||
|
||||
const address = settingField(dom, "Engine address");
|
||||
address!.value = "7.0.0.1:8765";
|
||||
dom.change(address as never);
|
||||
await settle();
|
||||
|
||||
const note = dom.find((n) => n.dataset["settingError"] === "baseUrl");
|
||||
assert.ok(note !== null && note.textContent !== "", `a bad address must be said:\n${dom.text()}`);
|
||||
assert.equal(note.hidden, false);
|
||||
assert.equal(
|
||||
note.textContent,
|
||||
en.settingRejectedExample("Engine address", "http://192.168.1.10:8765"),
|
||||
);
|
||||
// Naming the setting is not enough on this field: the user is looking at the
|
||||
// string they typed with no idea which part of it was wrong.
|
||||
assert.match(note.textContent, /http:\/\//, "the message must show a good address");
|
||||
// And the field is back to what is actually stored, not left holding a value
|
||||
// the app will never use.
|
||||
assert.equal(address!.value, "http://127.0.0.1:8765");
|
||||
await app?.dispose();
|
||||
});
|
||||
|
||||
test("a corrected address after a refusal is accepted, and clears the refusal", async () => {
|
||||
// The pair: a validator that refused everything would satisfy the test above
|
||||
// and make the one recovery path on a phone impossible.
|
||||
const { dom, platform } = harness();
|
||||
const app = await mount({ root: dom.root as never, platform, now: () => 1, newChatId: () => "c1" });
|
||||
await openSettings(dom);
|
||||
|
||||
const address = settingField(dom, "Engine address");
|
||||
address!.value = "7.0.0.1:8765";
|
||||
dom.change(address as never);
|
||||
await settle();
|
||||
address!.value = "http://10.0.0.7:9000/";
|
||||
dom.change(address as never);
|
||||
await settle();
|
||||
|
||||
const note = dom.find((n) => n.dataset["settingError"] === "baseUrl");
|
||||
assert.equal(note?.hidden, true, "an accepted write must clear its own refusal");
|
||||
// Normalised on the way in, so the trailing slash is not a second spelling of
|
||||
// the same engine.
|
||||
assert.equal(address!.value, "http://10.0.0.7:9000");
|
||||
await app?.dispose();
|
||||
});
|
||||
|
||||
test("Settings LEADS with the API key, not with the engine", async () => {
|
||||
// It was third of three, under a heading called "Engine" -- the wrong noun
|
||||
// for a credential, and the wrong order for the one field that decides
|
||||
// whether the app works at all.
|
||||
const { dom, platform } = harness();
|
||||
const app = await mount({ root: dom.root as never, platform, now: () => 1, newChatId: () => "c1" });
|
||||
await openSettings(dom);
|
||||
|
||||
const headings = dom.all().filter((n) => n.className === "settings-section");
|
||||
assert.deepEqual(headings.map((h) => h.textContent), ["API key", "Engine"]);
|
||||
assert.equal(headings[0]?.dataset["lead"], "true", "the first section is marked as the lead");
|
||||
assert.equal(headings[1]?.dataset["lead"], undefined);
|
||||
|
||||
// And the key's ROW comes before either engine row on the page.
|
||||
const rows = dom.all().filter((n) => n.className.includes("row-setting"));
|
||||
assert.deepEqual(rows.map((r) => r.dataset["settingKey"]), ["openaiApiKey", "engineId", "baseUrl"]);
|
||||
assert.equal(rows[0]?.dataset["lead"], "true");
|
||||
await app?.dispose();
|
||||
});
|
||||
|
||||
test("a setting's help text reaches the screen", async () => {
|
||||
// Every def has carried `help` since the registry was written and NOTHING
|
||||
// rendered it: the screen painted a label, a control and a refusal note and
|
||||
// dropped the sentence explaining what the field is for. That is the same
|
||||
// defect as an inert setting, one layer along -- the registry describes a
|
||||
// field the user never sees described.
|
||||
const { dom, platform } = harness();
|
||||
const app = await mount({ root: dom.root as never, platform, now: () => 1, newChatId: () => "c1" });
|
||||
await openSettings(dom);
|
||||
|
||||
const helps = dom.all().filter((n) => n.className === "setting-help");
|
||||
assert.equal(helps.length, SETTING_DEFS.filter((d) => d.help !== undefined).length);
|
||||
for (const def of SETTING_DEFS) {
|
||||
if (def.help === undefined) continue;
|
||||
assert.ok(
|
||||
helps.some((h) => h.textContent === def.help),
|
||||
`${def.key}'s help is declared and never painted:\n${dom.text()}`,
|
||||
);
|
||||
}
|
||||
await app?.dispose();
|
||||
});
|
||||
|
||||
test("a setting's help and inactive note are ASSOCIATED with the control, not just beside it", async () => {
|
||||
// Sitting a help paragraph next to a field tells a sighted user what it is
|
||||
// for; a screen-reader user who focuses the field hears only its aria-label
|
||||
// unless the control POINTS at the note. `aria-describedby` is that pointer.
|
||||
const dom = createFakeDom();
|
||||
// In-process engine: the address is inactive, so its "turn this on" sentence
|
||||
// must be announced with it, alongside the help every field carries.
|
||||
const off = paintSettings(dom, { engineId: ENGINE_PRAISONAI_TS }, new Map());
|
||||
dom.root.append(off as never);
|
||||
|
||||
const address = dom.find((n) => n.getAttribute("aria-label") === "Engine address");
|
||||
const describedBy = (address?.getAttribute("aria-describedby") ?? "").split(" ").filter(Boolean);
|
||||
const help = dom.find((n) => n.className === "setting-help" && n.id === "setting-help-baseUrl");
|
||||
const inactive = dom.find((n) => n.dataset["settingInactive"] === "baseUrl");
|
||||
assert.ok(help !== undefined && help.id !== "", "the help note needs a stable id to be pointed at");
|
||||
assert.ok(describedBy.includes(help!.id), "the address must describe itself with its help");
|
||||
assert.ok(
|
||||
describedBy.includes(inactive!.id),
|
||||
"and, while it is off, with the sentence that turns it on",
|
||||
);
|
||||
|
||||
// Live engine: the field applies, so the inactive id must NOT linger in the
|
||||
// description -- a screen reader would otherwise read a reason that is gone.
|
||||
const dom2 = createFakeDom();
|
||||
const on = paintSettings(dom2, { engineId: ENGINE_REMOTE_HTTP }, new Map());
|
||||
dom2.root.append(on as never);
|
||||
const live = dom2.find((n) => n.getAttribute("aria-label") === "Engine address");
|
||||
const liveDesc = (live?.getAttribute("aria-describedby") ?? "").split(" ").filter(Boolean);
|
||||
assert.ok(liveDesc.includes("setting-help-baseUrl"), "help stays whether or not the field applies");
|
||||
assert.ok(
|
||||
!liveDesc.includes("setting-inactive-baseUrl"),
|
||||
"a live field must not point at an inactive reason",
|
||||
);
|
||||
});
|
||||
|
||||
// The two tests above drive the screen through a CHANGE, which reaches the
|
||||
// sync walk. These two assert the FIRST PAINT, which is a different branch:
|
||||
// `buildSettingsScreen` decides what a row looks like before any event, and a
|
||||
// renderer that ignored `canClear` or `applies` there would be corrected a
|
||||
// moment later by the sync and pass everything above.
|
||||
|
||||
const paintSettings = (
|
||||
dom: ReturnType<typeof createFakeDom>,
|
||||
values: Readonly<Record<string, string>>,
|
||||
presence: Map<string, { ref: { slot: "openai"; account: string }; configured: boolean }>,
|
||||
) => {
|
||||
const store = createSettingsStore(SETTING_DEFS, createFakeStorage(), createFakeSecrets());
|
||||
const facade: SettingsFacade = {
|
||||
...facadeFor(store, createFakeSecrets()),
|
||||
get: (key) => values[key] ?? SETTING_DEFS.find((d) => d.key === key)?.default,
|
||||
};
|
||||
return buildSettingsScreen(dom.root.ownerDocument as never, facade, en, presence);
|
||||
};
|
||||
|
||||
test("Remove is not PAINTED for a key that is known to be missing", async () => {
|
||||
const dom = createFakeDom();
|
||||
const OPENAI = { slot: "openai" as const, account: "default" };
|
||||
|
||||
const missing = paintSettings(dom, {}, new Map([["openaiApiKey", { ref: OPENAI, configured: false }]]));
|
||||
dom.root.append(missing as never);
|
||||
const hiddenOne = dom.find((n) => n.dataset["action"] === "clear-secret");
|
||||
assert.equal(hiddenOne?.hidden, true, "a row painted as Not set must not paint a Remove");
|
||||
assert.equal(hiddenOne?.disabled, true);
|
||||
|
||||
// The pair, in the same test so a renderer cannot satisfy one by never
|
||||
// painting the button at all.
|
||||
const dom2 = createFakeDom();
|
||||
const present = paintSettings(dom2, {}, new Map([["openaiApiKey", { ref: OPENAI, configured: true }]]));
|
||||
dom2.root.append(present as never);
|
||||
const shown = dom2.find((n) => n.dataset["action"] === "clear-secret");
|
||||
assert.equal(shown?.hidden, false, "a row painted as Configured must offer Remove");
|
||||
assert.equal(shown?.disabled, false);
|
||||
});
|
||||
|
||||
test("the engine address is PAINTED switched off when the stored engine is in-process", async () => {
|
||||
// No `change` is fired here on purpose: this is what the user sees the
|
||||
// instant Settings opens, having chosen the in-process engine on a previous
|
||||
// launch. A renderer corrected only by the sync walk shows a live field until
|
||||
// something else is touched.
|
||||
const dom = createFakeDom();
|
||||
const off = paintSettings(dom, { engineId: ENGINE_PRAISONAI_TS }, new Map());
|
||||
dom.root.append(off as never);
|
||||
const address = dom.find((n) => n.getAttribute("aria-label") === "Engine address");
|
||||
assert.equal(address?.disabled, true, "nothing reads the address under the in-process engine");
|
||||
assert.equal(dom.find((n) => n.dataset["settingInactive"] === "baseUrl")?.hidden, false);
|
||||
|
||||
const dom2 = createFakeDom();
|
||||
const on = paintSettings(dom2, { engineId: ENGINE_REMOTE_HTTP }, new Map());
|
||||
dom2.root.append(on as never);
|
||||
assert.equal(
|
||||
dom2.find((n) => n.getAttribute("aria-label") === "Engine address")?.disabled,
|
||||
false,
|
||||
"and it is live the moment the remote engine is chosen",
|
||||
);
|
||||
assert.equal(dom2.find((n) => n.dataset["settingInactive"] === "baseUrl")?.hidden, true);
|
||||
});
|
||||
@@ -52,6 +52,7 @@ import {
|
||||
buildSettings,
|
||||
labelOf,
|
||||
presenceLabel,
|
||||
rowsOf,
|
||||
validateInput,
|
||||
type SecretPresence,
|
||||
type SecretRow,
|
||||
@@ -90,7 +91,12 @@ import {
|
||||
} from "../../engines/src/praisonai-ts/engine.ts";
|
||||
import { loadPraisonAgent, type PraisonAgentModule } from "../../engines/src/praisonai-ts/load-agent.ts";
|
||||
import type { PraisonAgent } from "../../engines/src/praisonai-ts/agent-api.ts";
|
||||
import { secretRefOf, type SettingDef, type SettingsFacade } from "../../core/src/settings/store.ts";
|
||||
import {
|
||||
appliesTo,
|
||||
secretRefOf,
|
||||
type SettingDef,
|
||||
type SettingsFacade,
|
||||
} from "../../core/src/settings/store.ts";
|
||||
import type { SecretsPort } from "../../core/src/ports/secrets.ts";
|
||||
import type { IgnoredReason } from "../../protocol/src/decode.ts";
|
||||
import type { HttpPort } from "../../core/src/ports/http.ts";
|
||||
@@ -285,9 +291,9 @@ function screenHeading(doc: Document, route: Route, strings: Strings): HTMLEleme
|
||||
* had no way to point it at one.
|
||||
*/
|
||||
function settingControl(doc: Document, def: SettingDef, row: ValueRow, settings: SettingsFacade): HTMLElement {
|
||||
let control: HTMLElement & { value: string };
|
||||
let control: HTMLElement & { value: string; disabled: boolean };
|
||||
if (row.control === "choice" && row.choices !== null) {
|
||||
const select = doc.createElement("select") as HTMLElement & { value: string };
|
||||
const select = doc.createElement("select") as HTMLElement & { value: string; disabled: boolean };
|
||||
for (const choice of row.choices) {
|
||||
const option = doc.createElement("option") as HTMLElement & { value: string };
|
||||
option.value = String(choice);
|
||||
@@ -296,7 +302,11 @@ function settingControl(doc: Document, def: SettingDef, row: ValueRow, settings:
|
||||
}
|
||||
control = select;
|
||||
} else {
|
||||
const input = doc.createElement("input") as HTMLElement & { value: string; type: string };
|
||||
const input = doc.createElement("input") as HTMLElement & {
|
||||
value: string;
|
||||
type: string;
|
||||
disabled: boolean;
|
||||
};
|
||||
input.type = row.control === "number" ? "number" : "text";
|
||||
control = input;
|
||||
}
|
||||
@@ -305,14 +315,90 @@ function settingControl(doc: Document, def: SettingDef, row: ValueRow, settings:
|
||||
control.value = String(settings.get(def.key) ?? def.default);
|
||||
control.className = "setting-value setting-input";
|
||||
control.setAttribute("aria-label", row.label);
|
||||
// Point the control at the notes that explain it: its help, and -- only while
|
||||
// it is inactive -- the sentence naming the switch that turns it on. Without
|
||||
// this a screen-reader user who focuses the field hears its label and nothing
|
||||
// of what it is for or why it is greyed out. `syncSettings` keeps the
|
||||
// inactive id in step as the field flips between applies/does-not.
|
||||
const describedBy: string[] = [];
|
||||
if (def.help !== undefined && def.help.trim() !== "") describedBy.push(helpId(def.key));
|
||||
if (!row.applies) describedBy.push(inactiveId(def.key));
|
||||
if (describedBy.length > 0) control.setAttribute("aria-describedby", describedBy.join(" "));
|
||||
// `change`, not `input`: a field commits on blur or Enter, so a half-typed
|
||||
// address is never stored and `set` is not called once per keystroke.
|
||||
// Delegated on root -- `change` bubbles.
|
||||
control.dataset["action"] = "set-setting";
|
||||
control.dataset["settingKey"] = def.key;
|
||||
// A field nothing reads is switched OFF rather than drawn as if it worked.
|
||||
// `baseUrl` accepted edits while the in-process engine was selected -- the
|
||||
// engine that runs on the device and never contacts an address -- so a value
|
||||
// typed there changed nothing and said nothing. `disabled` also keeps the
|
||||
// control out of the tab order and out of the accessibility tree's list of
|
||||
// things that can be operated, which a greyed-out colour alone would not.
|
||||
control.disabled = !row.applies;
|
||||
return control;
|
||||
}
|
||||
|
||||
/**
|
||||
* A def's `help`, on screen at last.
|
||||
*
|
||||
* Every def has carried one since the registry was written and NOTHING
|
||||
* rendered it: `buildSettingsScreen` painted the label, the control and the
|
||||
* refusal note, and dropped `row.help` on the floor. That is the same defect
|
||||
* as an inert setting one layer up -- a field the registry describes and the
|
||||
* screen does not -- and it is why the engine address could say "Only used by
|
||||
* the remote engine" in the source while the user saw a bare box.
|
||||
*/
|
||||
function settingHelp(doc: Document, key: string, help: string | null): HTMLElement | null {
|
||||
if (help === null || help.trim() === "") return null;
|
||||
const note = doc.createElement("p");
|
||||
note.className = "setting-help";
|
||||
note.id = helpId(key);
|
||||
note.textContent = help;
|
||||
return note;
|
||||
}
|
||||
|
||||
/** Stable id for a def's help note, so the control can point `aria-describedby`
|
||||
* at it. A screen reader that focuses a field otherwise hears its label and
|
||||
* nothing of what the field is for. */
|
||||
function helpId(key: string): string {
|
||||
return `setting-help-${key}`;
|
||||
}
|
||||
|
||||
/** Stable id for a def's inactive note ("Set Engine to remote-http to use
|
||||
* this"), so a disabled field announces WHY it is disabled rather than leaving
|
||||
* a screen-reader user to guess. */
|
||||
function inactiveId(key: string): string {
|
||||
return `setting-inactive-${key}`;
|
||||
}
|
||||
|
||||
/**
|
||||
* "Set Engine to remote-http to use this."
|
||||
*
|
||||
* Its own node, addressed by `data-setting-inactive`, for the same reason the
|
||||
* refusal note is: switching the engine has to be able to turn this sentence
|
||||
* on and off without rebuilding the screen and losing what is in the fields.
|
||||
* It is NOT `role="alert"` -- nothing has gone wrong, and announcing it would
|
||||
* interrupt the user in the middle of changing the very setting that produced
|
||||
* it.
|
||||
*/
|
||||
function settingInactiveNote(doc: Document, key: string): HTMLElement {
|
||||
const note = doc.createElement("p");
|
||||
note.className = "setting-inactive";
|
||||
note.id = inactiveId(key);
|
||||
note.dataset["settingInactive"] = key;
|
||||
note.hidden = true;
|
||||
return note;
|
||||
}
|
||||
|
||||
/** The sentence for a row that does not apply, or "" when it does. Shared by
|
||||
* the first paint and by every later `syncSettings`, so the two cannot drift
|
||||
* into saying different things about the same state. */
|
||||
function inactiveTextFor(row: ValueRow, strings: Strings): string {
|
||||
const because = row.inactiveBecause;
|
||||
return because === null ? "" : strings.settingInactive(because.label, because.value);
|
||||
}
|
||||
|
||||
/**
|
||||
* Where a refusal for one setting is written, and nothing until there is one.
|
||||
*
|
||||
@@ -377,7 +463,7 @@ function secretControls(doc: Document, row: SecretRow, strings: Strings): readon
|
||||
input.dataset["action"] = "set-secret";
|
||||
input.dataset["settingKey"] = row.key;
|
||||
|
||||
const clear = doc.createElement("button") as HTMLElement & { type: string };
|
||||
const clear = doc.createElement("button") as HTMLElement & { type: string; disabled: boolean };
|
||||
clear.type = "button";
|
||||
clear.className = "setting-clear";
|
||||
clear.textContent = strings.actionClearSecret;
|
||||
@@ -386,6 +472,15 @@ function secretControls(doc: Document, row: SecretRow, strings: Strings): readon
|
||||
// Named for the key it clears. "Remove" alone, repeated once per secret, is
|
||||
// a list of identical buttons to anyone navigating by control name.
|
||||
clear.setAttribute("aria-label", `${strings.actionClearSecret}: ${row.label}`);
|
||||
// OFFERED ONLY WHEN THERE IS SOMETHING TO REMOVE. It shipped unconditional,
|
||||
// sitting under a row that read "Not set" -- a control that destroys nothing
|
||||
// and reports nothing, which is the exact defect class this screen was meant
|
||||
// to be free of. `canClear` is false for UNKNOWN too, so the button does not
|
||||
// flash into existence and out again while the keychain lookup lands.
|
||||
// `disabled` as well as `hidden`: the click handler is delegated on root, so
|
||||
// hiding alone would leave a node that still answers a synthetic click.
|
||||
clear.hidden = !row.canClear;
|
||||
clear.disabled = !row.canClear;
|
||||
|
||||
return [presence, input, clear, settingError(doc, row.key)];
|
||||
}
|
||||
@@ -424,26 +519,42 @@ export function buildSettingsScreen(
|
||||
note.textContent = warning.text;
|
||||
section.append(note);
|
||||
}
|
||||
for (const group of view.sections) {
|
||||
for (const [index, group] of view.sections.entries()) {
|
||||
const title = doc.createElement("h3");
|
||||
title.className = "settings-section";
|
||||
// The first section is the one a new user came for, and it is marked as
|
||||
// such so the stylesheet can give it weight without the renderer having to
|
||||
// know WHICH section that is -- registry order decides, here as everywhere.
|
||||
if (index === 0) title.dataset["lead"] = "true";
|
||||
title.textContent = group.title;
|
||||
section.append(title);
|
||||
for (const row of group.rows) {
|
||||
const el = doc.createElement("div");
|
||||
el.className = `row row-setting row-setting-${row.kind}`;
|
||||
el.dataset["settingKey"] = row.key;
|
||||
// Rows in the lead section carry the mark too: the heading alone cannot
|
||||
// make the field under it read as the primary thing on the screen.
|
||||
if (index === 0) el.dataset["lead"] = "true";
|
||||
const label = doc.createElement("span");
|
||||
label.className = "setting-label";
|
||||
label.textContent = row.label;
|
||||
el.append(label);
|
||||
// Label, then what it is for, then the control. The help was declared on
|
||||
// every def and painted by nothing at all until now.
|
||||
const help = settingHelp(doc, row.key, row.help);
|
||||
if (help !== null) el.append(help);
|
||||
const def = row.kind === "value" ? defByKey.get(row.key) : undefined;
|
||||
if (row.kind === "value" && def !== undefined) {
|
||||
// The field and the place its refusal is written, together. The alert
|
||||
// node ships empty and hidden rather than being created on the failure
|
||||
// -- an alert region inserted at the moment it has something to say is
|
||||
// announced unreliably by every screen reader.
|
||||
el.append(settingControl(doc, def, row, settings), settingError(doc, row.key));
|
||||
// The field, the sentence that says which switch turns it on, and the
|
||||
// place its refusal is written. The alert node ships empty and hidden
|
||||
// rather than being created on the failure -- an alert region inserted
|
||||
// at the moment it has something to say is announced unreliably by
|
||||
// every screen reader.
|
||||
const inactive = settingInactiveNote(doc, row.key);
|
||||
const text = inactiveTextFor(row, strings);
|
||||
inactive.textContent = text;
|
||||
inactive.hidden = text === "";
|
||||
el.append(settingControl(doc, def, row, settings), inactive, settingError(doc, row.key));
|
||||
} else if (row.kind === "secret") {
|
||||
// Editable, at last. This row was a read-only `<span>` reporting
|
||||
// "Not set" forever: there was no secret def to render it and, had
|
||||
@@ -1355,7 +1466,7 @@ export async function mount(deps: MountDeps): Promise<App | null> {
|
||||
const refreshSecretPresence = (scope: HTMLElement): void => {
|
||||
const seq = ++latestPresenceSeq;
|
||||
void (async (): Promise<void> => {
|
||||
const answers = new Map<string, string>();
|
||||
const answers = new Map<string, boolean>();
|
||||
// Whether ANY credential is on file, which is the question the empty chat
|
||||
// screen asks. Resolved from the same walk rather than from a second one:
|
||||
// two independent lookups over the same keychain can land out of order
|
||||
@@ -1367,7 +1478,7 @@ export async function mount(deps: MountDeps): Promise<App | null> {
|
||||
if (ref === null) continue;
|
||||
const present = await app.settings.hasSecret(ref);
|
||||
anyPresent = anyPresent || present;
|
||||
answers.set(def.key, presenceLabel(present ? "configured" : "not-set"));
|
||||
answers.set(def.key, present);
|
||||
}
|
||||
// A newer refresh started while this one awaited: its answer is the
|
||||
// current truth, so this one must not overwrite it.
|
||||
@@ -1375,10 +1486,22 @@ export async function mount(deps: MountDeps): Promise<App | null> {
|
||||
keyPresence = anyPresent ? "present" : "absent";
|
||||
refreshEmptyState();
|
||||
for (const node of everyElement(scope)) {
|
||||
const key = node.dataset["secretPresence"];
|
||||
if (key === undefined) continue;
|
||||
const label = answers.get(key);
|
||||
if (label !== undefined) node.textContent = label;
|
||||
const presenceKey = node.dataset["secretPresence"];
|
||||
if (presenceKey !== undefined) {
|
||||
const configured = answers.get(presenceKey);
|
||||
if (configured !== undefined) {
|
||||
node.textContent = presenceLabel(configured ? "configured" : "not-set");
|
||||
}
|
||||
continue;
|
||||
}
|
||||
// Remove follows presence in the SAME pass. Two passes, or a rebuild,
|
||||
// is how a row comes to say "Not set" with a live Remove under it --
|
||||
// the state the screen shipped in permanently.
|
||||
if (node.dataset["action"] !== "clear-secret") continue;
|
||||
const configured = answers.get(node.dataset["settingKey"] ?? "");
|
||||
if (configured === undefined) continue;
|
||||
node.hidden = !configured;
|
||||
(node as HTMLElement & { disabled: boolean }).disabled = !configured;
|
||||
}
|
||||
})().catch(() => {
|
||||
// A keychain that will not answer leaves the row at UNKNOWN, which is
|
||||
@@ -1687,6 +1810,14 @@ export async function mount(deps: MountDeps): Promise<App | null> {
|
||||
*/
|
||||
const syncSettings = (key: string, refusal: string | null): void => {
|
||||
const defs = new Map(app.settings.defs().map((d) => [d.key, d]));
|
||||
// The CURRENT rows, built once, from the same function the first paint
|
||||
// used -- so "which switch turns this field on" is answered in one place
|
||||
// and the paint and the sync cannot drift into saying different things.
|
||||
const rows = new Map(
|
||||
rowsOf(buildSettings(app.settings))
|
||||
.filter((row): row is ValueRow => row.kind === "value")
|
||||
.map((row) => [row.key, row]),
|
||||
);
|
||||
const stack: HTMLElement[] = [root];
|
||||
while (stack.length > 0) {
|
||||
const node = stack.pop();
|
||||
@@ -1700,9 +1831,32 @@ export async function mount(deps: MountDeps): Promise<App | null> {
|
||||
(node as HTMLElement & { value: string }).value = String(
|
||||
app.settings.get(def.key) ?? def.default,
|
||||
);
|
||||
// Changing ONE setting can switch another one off: picking the
|
||||
// in-process engine is what makes the engine address stop meaning
|
||||
// anything. Recomputed here rather than on a rebuild, so the fields
|
||||
// keep what is in them while the dependency flips.
|
||||
const applies = appliesTo(def, (key) => app.settings.get(key));
|
||||
(node as HTMLElement & { disabled: boolean }).disabled = !applies;
|
||||
// Keep the description in step with the disabled state: a field that
|
||||
// has just gone inactive must POINT at the sentence that says why, and
|
||||
// one that has gone live must stop, or a screen reader keeps reading a
|
||||
// reason that no longer applies. Help stays whether or not it applies.
|
||||
const describedBy: string[] = [];
|
||||
if (def.help !== undefined && def.help.trim() !== "") describedBy.push(helpId(def.key));
|
||||
if (!applies) describedBy.push(inactiveId(def.key));
|
||||
if (describedBy.length > 0) node.setAttribute("aria-describedby", describedBy.join(" "));
|
||||
else node.removeAttribute("aria-describedby");
|
||||
}
|
||||
continue;
|
||||
}
|
||||
const inactiveFor = node.dataset["settingInactive"];
|
||||
if (inactiveFor !== undefined) {
|
||||
const row = rows.get(inactiveFor);
|
||||
const text = row === undefined ? "" : inactiveTextFor(row, strings);
|
||||
node.textContent = text;
|
||||
node.hidden = text === "";
|
||||
continue;
|
||||
}
|
||||
const errorFor = node.dataset["settingError"];
|
||||
if (errorFor === undefined) continue;
|
||||
// Only this key's note is touched. Clearing them all would wipe a
|
||||
@@ -1812,7 +1966,15 @@ export async function mount(deps: MountDeps): Promise<App | null> {
|
||||
// Said, not merely undone. A field that snaps back in silence is
|
||||
// indistinguishable from a mis-tap or from a save that worked, and on
|
||||
// this screen that leaves someone re-typing the same refused value.
|
||||
const refusal = stored ? null : strings.settingRejected(labelOf(def));
|
||||
// The example, when the def offers one. `baseUrl` is the field a user
|
||||
// gets wrong -- it now REFUSES `7.0.0.1:8765` instead of storing it,
|
||||
// and a refusal that only names the setting leaves them looking at the
|
||||
// same string with no idea which part of it was wrong.
|
||||
const refusal = stored
|
||||
? null
|
||||
: def.example === undefined
|
||||
? strings.settingRejected(labelOf(def))
|
||||
: strings.settingRejectedExample(labelOf(def), def.example);
|
||||
if (refusal !== null) assertive.textContent = refusal;
|
||||
syncSettings(intent.key, refusal);
|
||||
return;
|
||||
|
||||
@@ -16,6 +16,8 @@ import {
|
||||
CONSUMED_SETTING_KEYS,
|
||||
ENGINE_PRAISONAI_TS,
|
||||
ENGINE_REMOTE_HTTP,
|
||||
SECTION_API_KEY,
|
||||
SECTION_ENGINE,
|
||||
SETTING_DEFS,
|
||||
apiKeyFor,
|
||||
defaultEngineIdFor,
|
||||
@@ -655,3 +657,89 @@ test("the secret never reaches the plain settings file on the way through", asyn
|
||||
"the key appeared in the plain settings file",
|
||||
);
|
||||
});
|
||||
|
||||
// ---- the screen's information architecture ---------------------------------
|
||||
//
|
||||
// The one credential the app needs was filed under a heading called "Engine",
|
||||
// third of three rows, at the same weight as an engine picker and an address.
|
||||
// It is not an engine setting: it is the single field that decides whether the
|
||||
// app works at all. Registry order IS screen order (view-model.ts groups by
|
||||
// section in insertion order), so these two facts are asserted where they are
|
||||
// decided rather than through a DOM.
|
||||
|
||||
test("the API key is NOT filed under the engine heading", () => {
|
||||
const key = SETTING_DEFS.find((def) => def.key === "openaiApiKey");
|
||||
assert.ok(key !== undefined, "the app must still declare a credential");
|
||||
assert.notEqual(
|
||||
key.section,
|
||||
SECTION_ENGINE,
|
||||
"a credential is not an engine setting; it was filed under the wrong noun",
|
||||
);
|
||||
assert.equal(key.section, SECTION_API_KEY);
|
||||
// And the heading is not shared with anything else, so "API key" cannot come
|
||||
// to mean "and three other things".
|
||||
const others = SETTING_DEFS.filter((def) => def.section === SECTION_API_KEY && def.key !== key.key);
|
||||
assert.deepEqual(others.map((d) => d.key), []);
|
||||
});
|
||||
|
||||
test("the API key is the FIRST thing on the settings screen", () => {
|
||||
// Hierarchy, not just grouping. A new user opens Settings to paste a key;
|
||||
// making them read past two rows about runtimes first is the screen deciding
|
||||
// its own taxonomy matters more than the reason they came.
|
||||
assert.equal(
|
||||
SETTING_DEFS[0]?.key,
|
||||
"openaiApiKey",
|
||||
"the thing a new user needs most must be the thing they see first",
|
||||
);
|
||||
// Section order follows from row order, and that is the whole mechanism.
|
||||
const sections: string[] = [];
|
||||
for (const def of SETTING_DEFS) {
|
||||
const section = def.section ?? "";
|
||||
if (!sections.includes(section)) sections.push(section);
|
||||
}
|
||||
assert.deepEqual(sections, [SECTION_API_KEY, SECTION_ENGINE]);
|
||||
});
|
||||
|
||||
test("the engine address REFUSES an address with no scheme", () => {
|
||||
// `7.0.0.1:8765` was accepted in silence on a device. The def had no
|
||||
// `validate` at all, so the refusal machinery #4694 built had nothing to
|
||||
// refuse. Asserted on the DEF, because that is where the missing half was.
|
||||
const base = SETTING_DEFS.find((def) => def.key === "baseUrl");
|
||||
assert.ok(base?.validate !== undefined, "the engine address must be validated");
|
||||
assert.equal(base.validate("7.0.0.1:8765"), null);
|
||||
assert.equal(base.validate("http://192.168.1.10:8765"), "http://192.168.1.10:8765");
|
||||
// And the def says what a good one looks like, so the refusal can too.
|
||||
assert.ok(base.example !== undefined, "a def that refuses must be able to say what it wants");
|
||||
assert.equal(base.validate(base.example), base.example, "the example itself must be accepted");
|
||||
});
|
||||
|
||||
test("the engine address applies only while the remote engine is chosen", () => {
|
||||
// The field's own help said "Only used by the remote engine" while the field
|
||||
// rendered and accepted edits identically under the in-process engine, which
|
||||
// runs on the device and never contacts an address.
|
||||
const base = SETTING_DEFS.find((def) => def.key === "baseUrl");
|
||||
assert.deepEqual(base?.appliesWhen, { key: "engineId", equals: ENGINE_REMOTE_HTTP });
|
||||
// The setting it names has to exist, and to offer that value -- a dependency
|
||||
// on a key nobody declares switches the field off forever.
|
||||
const controller = SETTING_DEFS.find((def) => def.key === base?.appliesWhen?.key);
|
||||
assert.ok(controller !== undefined, "appliesWhen must name a declared setting");
|
||||
assert.ok(
|
||||
controller.choices?.includes(ENGINE_REMOTE_HTTP),
|
||||
"the value that switches the address on must be one the picker offers",
|
||||
);
|
||||
});
|
||||
|
||||
test("the engine's own default is one the address dependency recognises", () => {
|
||||
// On the web the app starts remote, so the address is live from first paint;
|
||||
// on a device it starts in-process, so it is not. Both are only true while
|
||||
// `defaultEngineIdFor` returns ids from the same closed set the dependency
|
||||
// compares against.
|
||||
for (const kind of ["web", "tauri"] as const) {
|
||||
const defs = settingDefsFor(kind);
|
||||
const engineId = defs.find((d) => d.key === "engineId")?.default;
|
||||
assert.ok(
|
||||
engineId === ENGINE_REMOTE_HTTP || engineId === ENGINE_PRAISONAI_TS,
|
||||
`${kind} defaults to an engine outside the picker: ${String(engineId)}`,
|
||||
);
|
||||
}
|
||||
});
|
||||
@@ -15,7 +15,12 @@ import type { AgentEnginePort } from "../../core/src/ports/agent-engine.ts";
|
||||
import type { HttpPort } from "../../core/src/ports/http.ts";
|
||||
import type { IgnoredReason } from "../../protocol/src/decode.ts";
|
||||
import type { SecretsPort } from "../../core/src/ports/secrets.ts";
|
||||
import { readSecretSetting, type SettingDef, type SettingsFacade } from "../../core/src/settings/store.ts";
|
||||
import {
|
||||
httpUrl,
|
||||
readSecretSetting,
|
||||
type SettingDef,
|
||||
type SettingsFacade,
|
||||
} from "../../core/src/settings/store.ts";
|
||||
import type { Platform } from "./platform.ts";
|
||||
import { createRemoteHttpEngine, probeHealth } from "../../engines/src/remote-http/engine.ts";
|
||||
import type { EngineChoice } from "./engines.ts";
|
||||
@@ -24,6 +29,17 @@ import type { ConversationHistory, RunPersistence } from "../../engines/src/prai
|
||||
export const ENGINE_REMOTE_HTTP = "remote-http";
|
||||
export const ENGINE_PRAISONAI_TS = "praisonai-ts";
|
||||
|
||||
/**
|
||||
* The headings, in the order they are rendered.
|
||||
*
|
||||
* Named constants rather than literals repeated on each def, so a section
|
||||
* cannot be split in two by a typo -- `buildSettings` groups by the STRING,
|
||||
* and "Engine " with a trailing space is a second heading with one row under
|
||||
* it. registry.test.ts asserts the credential's section is not the engine's.
|
||||
*/
|
||||
export const SECTION_API_KEY = "API key";
|
||||
export const SECTION_ENGINE = "Engine";
|
||||
|
||||
/**
|
||||
* What the app offers, with the metadata a screen needs to draw a control.
|
||||
*
|
||||
@@ -49,36 +65,19 @@ export const ENGINE_PRAISONAI_TS = "praisonai-ts";
|
||||
* empty" -- with no field anywhere in the app to put one in.
|
||||
*/
|
||||
export const SETTING_DEFS: readonly SettingDef[] = [
|
||||
{
|
||||
key: "engineId",
|
||||
default: ENGINE_REMOTE_HTTP,
|
||||
label: "Engine",
|
||||
help: "Which agent runtime answers. Remote talks to a PraisonAI engine over HTTP; in-process runs the agent loop on this device.",
|
||||
section: "Engine",
|
||||
// Exactly what `appEngines` in main.ts can build -- registry.test.ts
|
||||
// compares this list against that composition, because a picker offering
|
||||
// an id `selectEngine` rejects is a dead end: the choice persists, the
|
||||
// NEXT launch cannot build it, and boot dies at `renderFatal` with no way
|
||||
// back except editing storage by hand. That is what happened when
|
||||
// ENGINE_PRAISONAI_TS was listed here while nothing supplied
|
||||
// `createInProcess`. It is back because main.ts supplies the factory and
|
||||
// the engine ships as a chunk beside app.js (engines/praisonai-ts/
|
||||
// load-agent.ts).
|
||||
//
|
||||
// `default` here is the WEB default; `settingDefsFor` swaps in the
|
||||
// platform's, so the value Settings shows and the engine that answers a
|
||||
// first launch are the same one.
|
||||
choices: [ENGINE_REMOTE_HTTP, ENGINE_PRAISONAI_TS],
|
||||
},
|
||||
{
|
||||
key: "baseUrl",
|
||||
default: "http://127.0.0.1:8765",
|
||||
label: "Engine address",
|
||||
help: "Only used by the remote engine.",
|
||||
section: "Engine",
|
||||
},
|
||||
/**
|
||||
* The one credential the shipping app can actually use.
|
||||
* FIRST, and in a section of its own.
|
||||
*
|
||||
* It sat third, under a heading called "Engine", at the same visual weight
|
||||
* as two settings that matter less -- and it is the one field that decides
|
||||
* whether the app answers at all. A credential is not an engine setting: the
|
||||
* heading was the wrong noun for it, and a new user with a key in their
|
||||
* clipboard had to read past two rows about runtimes and addresses to find
|
||||
* the only row they came for.
|
||||
*
|
||||
* Registry order IS screen order (view-model.ts: "section order is REGISTRY
|
||||
* order: the order the author of the registry chose"), so moving it here is
|
||||
* the whole of the reordering -- there is no second list to keep in step.
|
||||
*
|
||||
* ONE slot, not five. `SecretSlot` is a closed union of openai | anthropic |
|
||||
* google | openrouter | custom, and it is closed for a keychain-namespace
|
||||
@@ -105,11 +104,65 @@ export const SETTING_DEFS: readonly SettingDef[] = [
|
||||
key: "openaiApiKey",
|
||||
default: "",
|
||||
label: "OpenAI API key",
|
||||
help: "Used by the in-process engine. Kept in the platform secret store, never in the settings file, and never shown back to you.",
|
||||
section: "Engine",
|
||||
help: "The in-process engine signs its requests with this. It is kept in the platform secret store, never in the settings file, and never shown back to you.",
|
||||
section: SECTION_API_KEY,
|
||||
secret: true,
|
||||
secretRef: { slot: "openai", account: "default" },
|
||||
},
|
||||
{
|
||||
key: "engineId",
|
||||
default: ENGINE_REMOTE_HTTP,
|
||||
label: "Engine",
|
||||
help: "Which agent runtime answers. Remote talks to a PraisonAI engine over HTTP; in-process runs the agent loop on this device.",
|
||||
section: SECTION_ENGINE,
|
||||
// Exactly what `appEngines` in main.ts can build -- registry.test.ts
|
||||
// compares this list against that composition, because a picker offering
|
||||
// an id `selectEngine` rejects is a dead end: the choice persists, the
|
||||
// NEXT launch cannot build it, and boot dies at `renderFatal` with no way
|
||||
// back except editing storage by hand. That is what happened when
|
||||
// ENGINE_PRAISONAI_TS was listed here while nothing supplied
|
||||
// `createInProcess`. It is back because main.ts supplies the factory and
|
||||
// the engine ships as a chunk beside app.js (engines/praisonai-ts/
|
||||
// load-agent.ts).
|
||||
//
|
||||
// `default` here is the WEB default; `settingDefsFor` swaps in the
|
||||
// platform's, so the value Settings shows and the engine that answers a
|
||||
// first launch are the same one.
|
||||
choices: [ENGINE_REMOTE_HTTP, ENGINE_PRAISONAI_TS],
|
||||
},
|
||||
{
|
||||
key: "baseUrl",
|
||||
default: "http://127.0.0.1:8765",
|
||||
label: "Engine address",
|
||||
help: "Where the remote engine listens. Include the scheme and the port.",
|
||||
section: SECTION_ENGINE,
|
||||
// The field's own help text used to say "Only used by the remote engine"
|
||||
// while the field rendered, and accepted edits, identically under the
|
||||
// in-process engine -- which runs on the device and never contacts an
|
||||
// address. Saying it in prose and denying it in the control is the worst
|
||||
// of both: the sentence is true and the screen contradicts it.
|
||||
//
|
||||
// DISABLED rather than hidden, and it is a close call. Hiding is tidier
|
||||
// and it is what a phone settings screen usually does. Two things decide
|
||||
// it the other way. A field that disappears takes its value off the screen
|
||||
// with it -- the store keeps it, so nothing is lost, but the user cannot
|
||||
// SEE that nothing is lost, and someone flipping between engines to
|
||||
// compare is watching their typed address vanish and reappear. And an
|
||||
// address is the thing a user goes looking for BEFORE they have chosen the
|
||||
// engine that uses it: a row that is not there cannot teach that the two
|
||||
// are connected, while a row that is there, greyed, and says "Set Engine
|
||||
// to remote-http to use this" teaches it in one glance.
|
||||
appliesWhen: { key: "engineId", equals: ENGINE_REMOTE_HTTP },
|
||||
// `7.0.0.1:8765` was accepted in silence: no scheme, no such host, no
|
||||
// error, and no way to find out until a turn failed. There was never a
|
||||
// validator here at all -- the refusal PATH has existed since #4694
|
||||
// (validateInput -> set -> the `role="alert"` note beside the field), and
|
||||
// nothing was ever handed to it to refuse. `httpUrl` is that missing half,
|
||||
// and it normalises as well as refuses, so one address is not stored three
|
||||
// ways.
|
||||
validate: httpUrl(),
|
||||
example: "http://192.168.1.10:8765",
|
||||
},
|
||||
];
|
||||
|
||||
/** The keys the shipping app actually reads. `engineId` in `boot.ts`, `baseUrl`
|
||||
|
||||
@@ -9,9 +9,11 @@ import test from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
|
||||
import {
|
||||
appliesTo,
|
||||
clampNum,
|
||||
createSettingsStore,
|
||||
facadeFor,
|
||||
httpUrl,
|
||||
plainOnly,
|
||||
readSecretSetting,
|
||||
secretRefOf,
|
||||
@@ -613,3 +615,119 @@ test("a secret never reaches the plain storage port UNDER ANY KEY", async () =>
|
||||
"the secret was not stored anywhere either; the loop proved nothing",
|
||||
);
|
||||
});
|
||||
|
||||
// ---- httpUrl: the validator `baseUrl` never had ------------------------------
|
||||
//
|
||||
// The field accepted `7.0.0.1:8765` -- no scheme, not a real address -- with no
|
||||
// validation, no error, and no way to discover the value was bad until a turn
|
||||
// failed with a transport error naming nothing the user had typed. The refusal
|
||||
// PATH has existed since #4694 (validateInput -> set -> a `role="alert"` note
|
||||
// beside the field); nothing was ever handed to it to refuse.
|
||||
|
||||
test("an address with no scheme is REFUSED, not stored", () => {
|
||||
const url = httpUrl();
|
||||
// The exact value the device accepted.
|
||||
assert.equal(url("7.0.0.1:8765"), null);
|
||||
assert.equal(url("127.0.0.1:8765"), null, "a bare host and port is not an address");
|
||||
assert.equal(url("engine.local"), null);
|
||||
assert.equal(url(""), null);
|
||||
assert.equal(url(" "), null);
|
||||
assert.equal(url("http://"), null, "a scheme with no host reaches nothing");
|
||||
});
|
||||
|
||||
test("a scheme that is not http is refused, including the ones that PARSE", () => {
|
||||
const url = httpUrl();
|
||||
// `localhost:8765` is a VALID URL -- protocol `localhost:`, path `8765` --
|
||||
// which is exactly why a bare `new URL()` in a try/catch is not enough. It is
|
||||
// also the near-miss a developer actually types.
|
||||
assert.equal(url("localhost:8765"), null);
|
||||
assert.equal(url("ftp://engine.local:8765"), null);
|
||||
assert.equal(url("javascript:alert(1)"), null);
|
||||
assert.equal(url("file:///etc/hosts"), null);
|
||||
});
|
||||
|
||||
test("a real address is accepted, and normalised so one engine is not three settings", () => {
|
||||
const url = httpUrl();
|
||||
assert.equal(url("http://192.168.1.10:8765"), "http://192.168.1.10:8765");
|
||||
assert.equal(url("https://engine.example.com"), "https://engine.example.com");
|
||||
// A trailing slash, surrounding whitespace and an uppercase host all name the
|
||||
// same engine. Stored apart they are three values that look different and
|
||||
// behave identically -- and every caller was doing its own `.replace()`.
|
||||
assert.equal(url(" http://192.168.1.10:8765/ "), "http://192.168.1.10:8765");
|
||||
assert.equal(url("http://192.168.1.10:8765//"), "http://192.168.1.10:8765");
|
||||
assert.equal(url("HTTP://Engine.Local:8765"), "http://engine.local:8765");
|
||||
// A path is kept: an engine behind a reverse proxy lives at one.
|
||||
assert.equal(url("http://proxy.example.com/praisonai/"), "http://proxy.example.com/praisonai");
|
||||
});
|
||||
|
||||
test("an address carrying a query or fragment is REFUSED, not stored to be mis-joined", () => {
|
||||
// The engine builds `${base}/chat` by concatenation, so a stored
|
||||
// "http://host:8765/?token=abc" becomes "http://host:8765/?token=abc/chat" --
|
||||
// a request for "/" with a mangled query, reaching none of the endpoints a
|
||||
// turn needs. Refuse at the point of acceptance rather than report a save
|
||||
// that cannot work.
|
||||
const url = httpUrl();
|
||||
assert.equal(url("http://192.168.1.10:8765?token=abc"), null, "a query has no place in an engine base");
|
||||
assert.equal(url("http://192.168.1.10:8765/?token=abc"), null);
|
||||
assert.equal(url("http://192.168.1.10:8765/#frag"), null, "nor does a fragment");
|
||||
assert.equal(url("http://proxy.example.com/praisonai?x=1"), null, "not even beside a real path");
|
||||
});
|
||||
|
||||
test("a non-string never reaches URL parsing", () => {
|
||||
const url = httpUrl();
|
||||
assert.equal(url(8765), null);
|
||||
assert.equal(url(true), null);
|
||||
});
|
||||
|
||||
test("a store REFUSES a malformed address rather than persisting it", () => {
|
||||
// The validator wired to the machinery that uses it: `set` runs `validate`,
|
||||
// and a null answer is a `false` the screen turns into a message. Without
|
||||
// this the two could be correct separately and never meet.
|
||||
const store = createSettingsStore(
|
||||
[{ key: "baseUrl", default: "http://127.0.0.1:8765", validate: httpUrl() }],
|
||||
createFakeStorage(),
|
||||
createFakeSecrets(),
|
||||
);
|
||||
return store.set("baseUrl", "7.0.0.1:8765").then(async (ok) => {
|
||||
assert.equal(ok, false, "a malformed address must be refused");
|
||||
assert.equal(store.get("baseUrl"), "http://127.0.0.1:8765", "and the old value must stand");
|
||||
assert.equal(await store.set("baseUrl", "http://10.0.0.7:9000"), true);
|
||||
assert.equal(store.get("baseUrl"), "http://10.0.0.7:9000");
|
||||
});
|
||||
});
|
||||
|
||||
test("a malformed address in a hand-edited settings file is dropped on load", () => {
|
||||
// `load` runs `validate` too. Without it the one path that skips the UI puts
|
||||
// back exactly the value the UI refuses.
|
||||
const storage = createFakeStorage();
|
||||
const secrets = createFakeSecrets();
|
||||
const defs: readonly SettingDef[] = [
|
||||
{ key: "baseUrl", default: "http://127.0.0.1:8765", validate: httpUrl() },
|
||||
];
|
||||
return storage
|
||||
.write({ namespace: "settings", id: "app" }, JSON.stringify({ baseUrl: "7.0.0.1:8765" }))
|
||||
.then(async () => {
|
||||
const store = createSettingsStore(defs, storage, secrets);
|
||||
await store.load();
|
||||
assert.equal(store.get("baseUrl"), "http://127.0.0.1:8765");
|
||||
assert.equal(store.isSet("baseUrl"), false, "a dropped value is not a chosen one");
|
||||
});
|
||||
});
|
||||
|
||||
// ---- appliesTo: a setting that depends on another ----------------------------
|
||||
|
||||
test("a setting applies unless its switch says otherwise", () => {
|
||||
const plain: SettingDef = { key: "model", default: "gpt-4o-mini" };
|
||||
const dependent: SettingDef = {
|
||||
key: "baseUrl",
|
||||
default: "http://127.0.0.1:8765",
|
||||
appliesWhen: { key: "engineId", equals: "remote-http" },
|
||||
};
|
||||
assert.equal(appliesTo(plain, () => undefined), true, "no dependency, always on");
|
||||
assert.equal(appliesTo(dependent, () => "remote-http"), true);
|
||||
assert.equal(appliesTo(dependent, () => "praisonai-ts"), false);
|
||||
// A controller the registry does not declare reads as undefined, which is not
|
||||
// the required value -- so the dependent setting is off rather than silently
|
||||
// treated as on.
|
||||
assert.equal(appliesTo(dependent, () => undefined), false);
|
||||
});
|
||||
@@ -15,6 +15,12 @@ import type { StoragePort } from "../ports/storage.ts";
|
||||
|
||||
export type SettingValue = string | number | boolean;
|
||||
|
||||
/** "This setting only does something while THAT one holds this value." */
|
||||
export interface SettingDependency {
|
||||
readonly key: string;
|
||||
readonly equals: SettingValue;
|
||||
}
|
||||
|
||||
export interface SettingDef {
|
||||
readonly key: string;
|
||||
readonly default: SettingValue;
|
||||
@@ -28,6 +34,32 @@ export interface SettingDef {
|
||||
/** A closed set of choices. Renders as a picker rather than a free field,
|
||||
* which is the difference between choosing a model and typing one. */
|
||||
readonly choices?: readonly SettingValue[];
|
||||
/**
|
||||
* A value that would be accepted, spelled out.
|
||||
*
|
||||
* Shown only when `validate` REFUSES what was typed. The refusal machinery
|
||||
* added in #4694 could say which setting was rejected and nothing about what
|
||||
* a good value looks like, so "Engine address was not changed" left the user
|
||||
* staring at the same field with the same wrong string in it. A def that can
|
||||
* refuse should be able to say what it wants instead; a def with no
|
||||
* `validate` has nothing to refuse and needs no example.
|
||||
*/
|
||||
readonly example?: string;
|
||||
/**
|
||||
* The other setting this one depends on, and the value that switches it on.
|
||||
*
|
||||
* Declared here rather than discovered by the screen, for the same reason
|
||||
* `secretRef` is: the registry is the only layer allowed to name a concrete
|
||||
* setting, and a view that hard-codes "baseUrl is dead unless engineId is
|
||||
* remote-http" is a view that goes stale the day a third engine lands.
|
||||
*
|
||||
* A setting that does not apply is still SHOWN -- disabled, and saying which
|
||||
* switch turns it on. Hiding it loses the value the user typed from the
|
||||
* screen (never from the store) and, worse, makes the field unfindable for
|
||||
* anyone who is looking for it before they have chosen the engine that uses
|
||||
* it.
|
||||
*/
|
||||
readonly appliesWhen?: SettingDependency;
|
||||
/** Secret values are routed to SecretsPort and never to StoragePort. */
|
||||
readonly secret?: boolean;
|
||||
/**
|
||||
@@ -301,6 +333,78 @@ export function facadeFor(store: SettingsStore, secrets: SecretsPort): SettingsF
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Does this setting do anything right now?
|
||||
*
|
||||
* True for a def with no `appliesWhen`, which is nearly all of them. The
|
||||
* dependency is read through `get`, not `isSet`: a def's DEFAULT is a live
|
||||
* value -- the engine really is remote-http on the web before anyone touches
|
||||
* the picker -- and asking `isSet` would report every dependent setting as
|
||||
* inactive until its controller had been changed at least once.
|
||||
*/
|
||||
export function appliesTo(
|
||||
def: SettingDef,
|
||||
read: (key: string) => SettingValue | undefined,
|
||||
): boolean {
|
||||
const dependency = def.appliesWhen;
|
||||
if (dependency === undefined) return true;
|
||||
return read(dependency.key) === dependency.equals;
|
||||
}
|
||||
|
||||
/**
|
||||
* An http(s) address, normalised -- or null.
|
||||
*
|
||||
* `baseUrl` shipped with no `validate` at all, so `7.0.0.1:8765` was stored,
|
||||
* displayed back, and discovered to be wrong only when a turn failed with a
|
||||
* transport error naming nothing the user had typed. The refusal machinery to
|
||||
* say so already existed (#4694): `validateInput` -> `set` -> a `role="alert"`
|
||||
* note beside the field. What was missing was anything that ever refused.
|
||||
*
|
||||
* Everything `new URL` will not parse is refused, and so is every scheme that
|
||||
* is not http or https -- `localhost:8765` PARSES, as a URL whose protocol is
|
||||
* `localhost:`, which is exactly the near-miss a user types. A bare host with
|
||||
* no scheme cannot parse at all when it starts with a digit and parses as a
|
||||
* scheme when it does not, so neither shape is let through.
|
||||
*
|
||||
* The stored value is the parsed one, not the typed one: a trailing slash, a
|
||||
* trailing space and an uppercase host all mean the same engine, and storing
|
||||
* them apart is three settings that look different and behave identically.
|
||||
* Trailing slashes go here rather than at the two call sites that were each
|
||||
* doing `.replace(/\/+$/, "")` on their own.
|
||||
*
|
||||
* A query string or fragment is refused, not carried through. The engine
|
||||
* builds `${base}/chat`, `${base}/health`, `${base}/approve/{id}` by plain
|
||||
* concatenation (engines/src/remote-http/engine.ts), so a stored
|
||||
* "http://host:8765/?token=abc" becomes "http://host:8765/?token=abc/chat" --
|
||||
* a request for "/" with a mangled query, not "/chat". The address the user is
|
||||
* told was saved reaches none of the endpoints a turn needs. `?` and `#` have
|
||||
* no place in an engine base, so they are refused at the point the value is
|
||||
* accepted rather than silently kept and mis-joined later.
|
||||
*/
|
||||
export function httpUrl(): (value: SettingValue) => SettingValue | null {
|
||||
return (value) => {
|
||||
if (typeof value !== "string") return null;
|
||||
const text = value.trim();
|
||||
if (text === "") return null;
|
||||
let url: URL;
|
||||
try {
|
||||
url = new URL(text);
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
if (url.protocol !== "http:" && url.protocol !== "https:") return null;
|
||||
if (url.hostname === "") return null;
|
||||
// A base the endpoints append to cannot carry a query or fragment: `?x` and
|
||||
// `#y` would land BEFORE the `/chat` the engine concatenates, addressing "/"
|
||||
// with a broken query instead of the endpoint. Refuse rather than keep-and-
|
||||
// mis-join.
|
||||
if (url.search !== "" || url.hash !== "") return null;
|
||||
const normalised = `${url.origin}${url.pathname}`.replace(/\/+$/, "");
|
||||
// `origin` alone for a bare "http://host/", which strips to "http://host".
|
||||
return normalised === "" ? url.origin : normalised;
|
||||
};
|
||||
}
|
||||
|
||||
/** Clamp a number into range. Lifted from the desktop settings registry, where
|
||||
* it is a named function precisely so a test can call it. */
|
||||
export function clampNum(min: number, max: number): (value: SettingValue) => SettingValue | null {
|
||||
|
||||
@@ -22,6 +22,9 @@ const SAMPLES: Readonly<Record<string, () => string>> = {
|
||||
bootFailed: () => en.bootFailed("no storage"),
|
||||
engineNotReady: () => en.engineNotReady("ECONNREFUSED"),
|
||||
settingRejected: () => en.settingRejected("Engine address"),
|
||||
settingRejectedExample: () =>
|
||||
en.settingRejectedExample("Engine address", "http://192.168.1.10:8765"),
|
||||
settingInactive: () => en.settingInactive("Engine", "remote-http"),
|
||||
secretStored: () => en.secretStored("OpenAI API key"),
|
||||
secretCleared: () => en.secretCleared("OpenAI API key"),
|
||||
chatUnreadable: () => en.chatUnreadable("chat-7"),
|
||||
|
||||
@@ -89,6 +89,27 @@ export interface Strings {
|
||||
* may already have scrolled off.
|
||||
*/
|
||||
readonly settingRejected: (label: string) => string;
|
||||
/**
|
||||
* The same refusal, plus a value that WOULD be accepted.
|
||||
*
|
||||
* `settingRejected` names the setting and stops there, which is the whole of
|
||||
* what it can say for a def whose `validate` is a clamp. For a def that
|
||||
* declares an `example`, stopping there leaves the user looking at the field
|
||||
* they just typed into with no idea which part of it was wrong -- and the
|
||||
* engine address is exactly that field: `7.0.0.1:8765` and
|
||||
* `http://192.168.1.10:8765` differ in a scheme, and nothing on the screen
|
||||
* said a scheme was expected.
|
||||
*/
|
||||
readonly settingRejectedExample: (label: string, example: string) => string;
|
||||
/**
|
||||
* A field that is switched off, and the switch that turns it on.
|
||||
*
|
||||
* Not "This field is disabled" -- that describes the screen back to the
|
||||
* person looking at it. It names the OTHER setting and the value to put it
|
||||
* at, so the sentence is an instruction the reader can carry out on the same
|
||||
* screen without scrolling.
|
||||
*/
|
||||
readonly settingInactive: (label: string, value: string) => string;
|
||||
/**
|
||||
* A secret field's placeholder, and its accessible name.
|
||||
*
|
||||
@@ -379,6 +400,8 @@ export const en: Strings = {
|
||||
// neither the setting nor the outcome. "was not changed" is the fact the
|
||||
// user needs: the old value is still in force.
|
||||
settingRejected: (label) => `${label} was not changed: that value was refused.`,
|
||||
settingRejectedExample: (label, example) => `${label} was not changed. Enter it like ${example}.`,
|
||||
settingInactive: (label, value) => `Set ${label} to ${value} to use this.`,
|
||||
// "Paste" rather than "Enter": on a phone nobody types an API key, and the
|
||||
// instruction that matches what the user is actually about to do is the one
|
||||
// that reads as written for them.
|
||||
|
||||
@@ -313,3 +313,94 @@ test("the software-secrets warning says where the key really is", () => {
|
||||
// being shown on a device, which is the state this change ended.
|
||||
assert.doesNotMatch(text, /on this platform/i, "there is only one platform that shows this now");
|
||||
});
|
||||
|
||||
// ---- rule 5: a control that cannot do anything says so ----------------------
|
||||
//
|
||||
// Two of these shipped on the same screen. `Remove` sat under a row reading
|
||||
// "Not set", and `Engine address` rendered identically whether or not an engine
|
||||
// that reads it was selected -- while its own help text said "Only used by the
|
||||
// remote engine". Both answers are computed here rather than in the renderer,
|
||||
// so a second renderer cannot get a different one and a test does not need a
|
||||
// DOM to ask.
|
||||
|
||||
test("Remove is offered only when there is a key to remove", () => {
|
||||
// The defect, exactly: the button was unconditional, so a user with no key
|
||||
// was offered a control that destroys nothing and reports nothing. It is the
|
||||
// defect class this package spent two days eliminating, in the screen that
|
||||
// introduced it.
|
||||
const configured: SecretPresence = new Map([["apiKey", { ref: OPENAI, configured: true }]]);
|
||||
const missing: SecretPresence = new Map([["apiKey", { ref: OPENAI, configured: false }]]);
|
||||
|
||||
const withKey = secretRowsOf(buildSettings(fakeFacade(DEFS), configured))[0];
|
||||
const withoutKey = secretRowsOf(buildSettings(fakeFacade(DEFS), missing))[0];
|
||||
|
||||
assert.equal(withKey?.canClear, true, "a stored key must be removable");
|
||||
assert.equal(withoutKey?.canClear, false, "there is nothing to remove");
|
||||
// And the presence word and the button agree. Two sources for one fact is
|
||||
// how a row comes to say "Not set" with a live Remove under it.
|
||||
assert.equal(withoutKey?.presence, NOT_SET);
|
||||
});
|
||||
|
||||
test("a key whose lookup has not landed offers no Remove either -- the pair", () => {
|
||||
// `canClear` is `state === "configured"`, not `state !== "not-set"`. UNKNOWN
|
||||
// is a keychain check still in flight (rule 2), and offering to destroy a
|
||||
// credential the app has not confirmed exists is the same false promise
|
||||
// pointed the other way -- and the state EVERY row is in on first paint.
|
||||
const row = secretRowsOf(buildSettings(fakeFacade(DEFS)))[0];
|
||||
assert.equal(row?.presence, UNKNOWN);
|
||||
assert.equal(row?.canClear, false, "an unresolved lookup must not offer Remove");
|
||||
});
|
||||
|
||||
const DEPENDENT: readonly SettingDef[] = [
|
||||
{ key: "engineId", default: "remote-http", label: "Engine", section: "Engine", choices: ["remote-http", "praisonai-ts"] },
|
||||
{
|
||||
key: "baseUrl",
|
||||
default: "http://127.0.0.1:8765",
|
||||
label: "Engine address",
|
||||
section: "Engine",
|
||||
appliesWhen: { key: "engineId", equals: "remote-http" },
|
||||
},
|
||||
];
|
||||
|
||||
test("a setting whose switch is off does not apply, and names the switch", () => {
|
||||
const rows = rowsOf(buildSettings(fakeFacade(DEPENDENT, { engineId: "praisonai-ts" })));
|
||||
const row = value(rows, "baseUrl");
|
||||
assert.equal(row.kind, "value");
|
||||
assert.equal(row.kind === "value" && row.applies, false, "nothing reads it under this engine");
|
||||
// The LABEL of the controlling setting, not its key: a user sent hunting for
|
||||
// `engineId` on a screen that only ever says "Engine" has been sent nowhere.
|
||||
assert.deepEqual(row.kind === "value" ? row.inactiveBecause : null, {
|
||||
label: "Engine",
|
||||
value: "remote-http",
|
||||
});
|
||||
});
|
||||
|
||||
test("the same setting applies once its switch is on -- the pair", () => {
|
||||
// Without this, a row hard-wired to `applies: false` would satisfy the test
|
||||
// above and the address would be permanently uneditable -- which is the
|
||||
// recovery path a phone depends on (#4694).
|
||||
const rows = rowsOf(buildSettings(fakeFacade(DEPENDENT, { engineId: "remote-http" })));
|
||||
const row = value(rows, "baseUrl");
|
||||
assert.equal(row.kind === "value" && row.applies, true);
|
||||
assert.equal(row.kind === "value" ? row.inactiveBecause : "x", null);
|
||||
});
|
||||
|
||||
test("a setting with no dependency always applies", () => {
|
||||
// Nearly every def. A default of `applies: false` would switch the whole
|
||||
// screen off at once.
|
||||
for (const row of rowsOf(buildSettings(fakeFacade(DEFS)))) {
|
||||
if (row.kind !== "value") continue;
|
||||
assert.equal(row.applies, true, `${row.key} has no dependency and must apply`);
|
||||
}
|
||||
});
|
||||
|
||||
test("a dependency the DEFAULT already satisfies applies before anyone touches it", () => {
|
||||
// Read through `get` and not `isSet`: a def's default is a live value -- the
|
||||
// engine really is remote-http on first launch -- so asking whether the
|
||||
// controlling setting had been CHANGED would report the address inactive on
|
||||
// a screen where it is the only thing that works.
|
||||
const rows = rowsOf(buildSettings(fakeFacade(DEPENDENT)));
|
||||
assert.equal(value(rows, "baseUrl").kind === "value", true);
|
||||
const row = value(rows, "baseUrl");
|
||||
assert.equal(row.kind === "value" && row.applies, true, "the default engine is the remote one");
|
||||
});
|
||||
@@ -35,12 +35,27 @@
|
||||
* the control from it means one bad write turns a toggle into a text box
|
||||
* and the setting can no longer be changed back.
|
||||
*
|
||||
* 5. A CONTROL THAT CANNOT DO ANYTHING IS SAID TO BE OFF, NOT DRAWN AS IF IT
|
||||
* WERE ON. Two of them shipped. `Remove` sat under a key that read "Not
|
||||
* set", so the one control on the row destroyed nothing and reported
|
||||
* nothing. `Engine address` rendered identically whether or not an engine
|
||||
* that reads it was selected -- its own help text said "Only used by the
|
||||
* remote engine" while the field beside it accepted edits that changed
|
||||
* what nothing read. `canClear` and `applies` are computed HERE, from the
|
||||
* same presence and the same registry the rest of the row comes from, so a
|
||||
* renderer cannot forget one and a test can assert both without a DOM.
|
||||
*
|
||||
* Row ids are derived from the setting key, never from array position, so a
|
||||
* keyed renderer (render/reconcile.ts) cannot paint one row's contents into
|
||||
* another row's node when a section gains an entry.
|
||||
*/
|
||||
import type { SecretRef } from "../../../core/src/ports/secrets.ts";
|
||||
import type { SettingDef, SettingValue, SettingsFacade } from "../../../core/src/settings/store.ts";
|
||||
import {
|
||||
appliesTo,
|
||||
type SettingDef,
|
||||
type SettingValue,
|
||||
type SettingsFacade,
|
||||
} from "../../../core/src/settings/store.ts";
|
||||
import { UNKNOWN } from "../format.ts";
|
||||
|
||||
/** Where a def with no `section` lands. Named so a test and a renderer agree. */
|
||||
@@ -84,6 +99,28 @@ export interface ValueRow {
|
||||
readonly value: SettingValue;
|
||||
/** The closed set for a `choice` control, null for every other kind. */
|
||||
readonly choices: readonly SettingValue[] | null;
|
||||
/**
|
||||
* Does anything read this setting right now?
|
||||
*
|
||||
* False when the def's `appliesWhen` names another setting that does not
|
||||
* currently hold the required value. The row is still rendered -- see rule 5
|
||||
* -- but as a field that is switched off rather than as one that works.
|
||||
*/
|
||||
readonly applies: boolean;
|
||||
/**
|
||||
* The switch that would turn it on, in the words the screen already uses for
|
||||
* it, or null when the row applies (or names a setting the registry does not
|
||||
* declare). A raw key here would send the user hunting for `engineId` on a
|
||||
* screen that only ever says "Engine".
|
||||
*/
|
||||
readonly inactiveBecause: InactiveBecause | null;
|
||||
}
|
||||
|
||||
/** Which other setting switches this one on, and to what. Both already
|
||||
* rendered as text elsewhere on the screen, so both are strings. */
|
||||
export interface InactiveBecause {
|
||||
readonly label: string;
|
||||
readonly value: string;
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -104,6 +141,17 @@ export interface SecretRow {
|
||||
/** Where to write it. Null when nothing has told this view which slot the
|
||||
* def belongs to -- see the note on `secretPresence`. */
|
||||
readonly ref: SecretRef | null;
|
||||
/**
|
||||
* Is there anything for `Remove` to remove?
|
||||
*
|
||||
* ONLY `configured`. Not "not `not-set`" -- `unknown` is a check still in
|
||||
* flight (rule 2), and offering to destroy a credential the app has not yet
|
||||
* confirmed exists is the same false promise in the other direction. The
|
||||
* screen shipped with an unconditional `Remove` under a row reading "Not
|
||||
* set": a control that cannot do anything, which is the defect class this
|
||||
* package spent two days removing everywhere else.
|
||||
*/
|
||||
readonly canClear: boolean;
|
||||
/** Structurally unfillable. Do not remove. */
|
||||
readonly value?: never;
|
||||
}
|
||||
@@ -218,12 +266,23 @@ function secretRow(def: SettingDef, presence: SecretPresence): SecretRow {
|
||||
state,
|
||||
presence: presenceLabel(state),
|
||||
ref: status?.ref ?? null,
|
||||
// A ref is required as well as presence: `clearSecret` needs somewhere to
|
||||
// clear, and a row that is "configured" with nowhere to write is a row
|
||||
// whose button would resolve to nothing.
|
||||
canClear: state === "configured" && status?.ref !== undefined,
|
||||
};
|
||||
}
|
||||
|
||||
function valueRow(def: SettingDef, facade: SettingsFacade): ValueRow {
|
||||
function valueRow(
|
||||
def: SettingDef,
|
||||
facade: SettingsFacade,
|
||||
byKey: ReadonlyMap<string, SettingDef>,
|
||||
): ValueRow {
|
||||
const stored = facade.get(def.key);
|
||||
const control = controlFor(def);
|
||||
const applies = appliesTo(def, (key) => facade.get(key));
|
||||
const dependency = def.appliesWhen;
|
||||
const controller = dependency === undefined ? undefined : byKey.get(dependency.key);
|
||||
return {
|
||||
kind: "value",
|
||||
id: `setting:${def.key}`,
|
||||
@@ -236,6 +295,11 @@ function valueRow(def: SettingDef, facade: SettingsFacade): ValueRow {
|
||||
// user switched off that switches itself back on when the screen reopens.
|
||||
value: stored ?? def.default,
|
||||
choices: control === "choice" ? (def.choices ?? null) : null,
|
||||
applies,
|
||||
inactiveBecause:
|
||||
applies || dependency === undefined || controller === undefined
|
||||
? null
|
||||
: { label: labelOf(controller), value: String(dependency.equals) },
|
||||
};
|
||||
}
|
||||
|
||||
@@ -255,6 +319,9 @@ export function buildSettings(
|
||||
// reshuffle the screen the day a section is renamed.
|
||||
const sections = new Map<string, SettingsRow[]>();
|
||||
const seen = new Set<string>();
|
||||
// Resolved once. A dependent row names its controller by LABEL, and looking
|
||||
// that up per row would be a scan of the whole registry per row.
|
||||
const byKey = new Map(facade.defs().map((def) => [def.key, def]));
|
||||
|
||||
for (const def of facade.defs()) {
|
||||
// Defensive: two defs sharing a key would produce two rows with the same
|
||||
@@ -265,7 +332,9 @@ export function buildSettings(
|
||||
|
||||
const title = def.section?.trim() === "" ? GENERAL_SECTION : (def.section ?? GENERAL_SECTION);
|
||||
const rows = sections.get(title) ?? [];
|
||||
rows.push(def.secret === true ? secretRow(def, secretPresence) : valueRow(def, facade));
|
||||
rows.push(
|
||||
def.secret === true ? secretRow(def, secretPresence) : valueRow(def, facade, byKey),
|
||||
);
|
||||
sections.set(title, rows);
|
||||
}
|
||||
|
||||
|
||||
Reference in new issue
Block a user