mirror of
https://github.com/block/buzz.git
synced 2026-08-18 06:50:31 +02:00
fix(desktop): stop the create-agent provider config probe from erasing keystrokes (#4411)
## What
Fixes the create-agent dialog's "Run on" provider config fields eating
keystrokes — reported by Tyler in buzz-remote-agents (channel
`29414326`, thread `db76677a`): the Kubernetes **Kubeconfig context**
field would not accept typing.
## Why it happened (the Typewriter Eraser, shipped in #4289)
`WhereToRunSection`'s probe `useEffect` depended on the whole `draft`:
1. every keystroke changed the draft → effect re-fired → provider binary
re-probed;
2. each probe result is a fresh object written into the draft → the
effect re-triggered **itself**, respawning the provider binary in a loop
for as long as the dialog sat on a provider;
3. every probe resolution reset `providerConfig` to schema defaults —
erasing whatever was typed. A field with no schema default (`context`)
snapped back to empty, i.e. "won't let me type". Unrelated to how many
kubeconfig contexts you have.
## Fix
- **Probe once per provider selection**, keyed on the provider's stable
`binaryPath` — not the draft, not the provider object (a
`useBackendProvidersQuery` refresh must not reprobe an unchanged
selection).
- **Latest-state resolution** via `React.useEffectEvent` + a new pure
`applyProbeResult` helper: schema defaults merge **beneath** the current
`providerConfig`, so a probe landing after the user typed can never
clobber in-flight input (per Wren's pre-patch red-team: changing deps
alone leaves a stale closure).
Existing `cancelled` cleanup keeps provider-switch/unmount safe;
selection reset (`emptyWhereToRunDraft`) and the fail-closed probe-error
path are unchanged.
## Tests
- **Unit** (`whereToRunIntent.test.mjs`): `applyProbeResult` merge
semantics — defaults under typed values, user-cleared fields stay
cleared, schema-less results, unrelated fields preserved.
- **E2E** (new `where-to-run-config.spec.ts`, added to the smoke
project, **red-first verified**: all 3 fail against the unfixed
component):
- typing into a defaultless provider field sticks, and
`probe_backend_provider` fires exactly once per selection;
- the config form is gated on probe resolution (slow probe: no
half-rendered form, defaults prefill once);
- provider → local → provider re-probes and resets cleanly.
- Mock bridge gains `backendProviders` / `backendProviderProbeResult` /
`backendProviderProbeDelayMs` seams (defaults preserve prior behavior).
## Verification at 8eb7680
- `pnpm check` + `tsc` clean, `pnpm test` 3926/3926;
- new spec 3/3 green (and 3/3 red on the unfixed component);
- pre-push lefthook: desktop-test, desktop-check, desktop-tauri-checks,
rust-tests, mobile-test all green.
---------
Signed-off-by: npub1qyvc0c5kl4gqv2fd97fsk46tu378sqgy35vc83rvgfwne90sel7s0ed67d <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz>
Co-authored-by: npub1qyvc0c5kl4gqv2fd97fsk46tu378sqgy35vc83rvgfwne90sel7s0ed67d <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz>
This commit is contained in:
co-authored by
npub1qyvc0c5kl4gqv2fd97fsk46tu378sqgy35vc83rvgfwne90sel7s0ed67d
parent
a5dbdf5e61
commit
2c0ac24674
@@ -132,6 +132,7 @@ export default defineConfig({
|
||||
"**/harness-management.spec.ts",
|
||||
"**/harness-catalog-screenshots.spec.ts",
|
||||
"**/inline-custom-harness.spec.ts",
|
||||
"**/where-to-run-config.spec.ts",
|
||||
"**/huddle-transcription.spec.ts",
|
||||
],
|
||||
use: {
|
||||
|
||||
@@ -5,7 +5,11 @@ import { useBackendProvidersQuery } from "@/features/agents/hooks";
|
||||
import { probeBackendProvider } from "@/shared/api/tauri";
|
||||
|
||||
import { ProviderConfigFields } from "./ProviderConfigFields";
|
||||
import { emptyWhereToRunDraft, type WhereToRunDraft } from "./whereToRunIntent";
|
||||
import {
|
||||
applyProbeResult,
|
||||
emptyWhereToRunDraft,
|
||||
type WhereToRunDraft,
|
||||
} from "./whereToRunIntent";
|
||||
|
||||
/** Optional remote-backend selector. Buzz shared compute is an LLM provider, not a run destination. */
|
||||
export function WhereToRunSection({
|
||||
@@ -26,32 +30,37 @@ export function WhereToRunSection({
|
||||
[backendProviders, draft.runOn],
|
||||
);
|
||||
|
||||
// Latest-state seam for probe resolution: an Effect Event always sees the
|
||||
// draft as it is *now*. Without this, the probe promise closes over the
|
||||
// draft from probe start, and anything typed while the probe was in flight
|
||||
// gets thrown away when it resolves (a second, subtler Typewriter Eraser).
|
||||
const applyProbe = React.useEffectEvent(
|
||||
(result: Awaited<ReturnType<typeof probeBackendProvider>>) => {
|
||||
onDraftChange(applyProbeResult(draft, result));
|
||||
},
|
||||
);
|
||||
|
||||
// Probe once per provider *selection*, keyed on the provider's stable
|
||||
// path — never on the draft. Depending on the draft made every keystroke
|
||||
// refire the probe, and each resolution reset providerConfig to schema
|
||||
// defaults, which erased what the user was typing (the Typewriter Eraser)
|
||||
// and spawned the provider binary in a loop for as long as the dialog was
|
||||
// open. Keying on the path (not the provider object) also keeps a
|
||||
// providers-query refresh from reprobing an unchanged selection.
|
||||
const selectedBinaryPath = isProviderMode
|
||||
? (selectedBackendProvider?.binaryPath ?? null)
|
||||
: null;
|
||||
React.useEffect(() => {
|
||||
if (!isProviderMode || !selectedBackendProvider) {
|
||||
if (!selectedBinaryPath) {
|
||||
setProbeError(null);
|
||||
return;
|
||||
}
|
||||
let cancelled = false;
|
||||
setProbeError(null);
|
||||
void probeBackendProvider(selectedBackendProvider.binaryPath)
|
||||
void probeBackendProvider(selectedBinaryPath)
|
||||
.then((result) => {
|
||||
if (cancelled) return;
|
||||
const defaults: Record<string, string> = {};
|
||||
const properties =
|
||||
(result.config_schema as Record<string, unknown> | undefined)
|
||||
?.properties ?? {};
|
||||
for (const [key, property] of Object.entries(properties) as [
|
||||
string,
|
||||
Record<string, unknown>,
|
||||
][]) {
|
||||
if (property.default != null)
|
||||
defaults[key] = String(property.default);
|
||||
}
|
||||
onDraftChange({
|
||||
...draft,
|
||||
probedProvider: result,
|
||||
providerConfig: defaults,
|
||||
});
|
||||
applyProbe(result);
|
||||
})
|
||||
.catch((error: unknown) => {
|
||||
if (!cancelled) {
|
||||
@@ -61,7 +70,7 @@ export function WhereToRunSection({
|
||||
return () => {
|
||||
cancelled = true;
|
||||
};
|
||||
}, [draft, isProviderMode, onDraftChange, selectedBackendProvider]);
|
||||
}, [selectedBinaryPath]);
|
||||
|
||||
if (backendProviders.length === 0) return null;
|
||||
|
||||
|
||||
@@ -2,6 +2,7 @@ import assert from "node:assert/strict";
|
||||
import test from "node:test";
|
||||
|
||||
import {
|
||||
applyProbeResult,
|
||||
canSubmitWhereToRun,
|
||||
emptyWhereToRunDraft,
|
||||
providerConfigComplete,
|
||||
@@ -59,3 +60,72 @@ test("provider draft resolves with coerced config values", () => {
|
||||
config: { region: "us", size: 3 },
|
||||
});
|
||||
});
|
||||
|
||||
// ── applyProbeResult: probe resolution must merge, not overwrite ─────────────
|
||||
//
|
||||
// Pins the seam that fixed the "Typewriter Eraser" (agent-create dialog's
|
||||
// provider config fields losing keystrokes): a probe resolution prefills
|
||||
// schema defaults *beneath* the user's in-flight config, never over it. The
|
||||
// effect in WhereToRunSection keys probing on the provider's binary path, so
|
||||
// the only probe writes that reach providerConfig are the ones pinned here.
|
||||
|
||||
const probeWithDefaults = {
|
||||
ok: true,
|
||||
config_schema: {
|
||||
properties: {
|
||||
context: { type: "string", title: "Kubeconfig context" },
|
||||
namespace: { type: "string", default: "buzz-agents-x1y2z3" },
|
||||
inactivity_seconds: { type: "number", default: 1800 },
|
||||
},
|
||||
required: ["namespace"],
|
||||
},
|
||||
};
|
||||
|
||||
const unprobedDraft = {
|
||||
...emptyWhereToRunDraft,
|
||||
runOn: "kubernetes",
|
||||
};
|
||||
|
||||
test("probe resolution prefills schema defaults on a fresh draft", () => {
|
||||
const next = applyProbeResult(unprobedDraft, probeWithDefaults);
|
||||
assert.equal(next.probedProvider, probeWithDefaults);
|
||||
assert.deepEqual(next.providerConfig, {
|
||||
namespace: "buzz-agents-x1y2z3",
|
||||
inactivity_seconds: "1800",
|
||||
});
|
||||
});
|
||||
|
||||
test("probe resolution keeps user-typed values over schema defaults", () => {
|
||||
const typed = {
|
||||
...unprobedDraft,
|
||||
providerConfig: { context: "prod-us-west", namespace: "my-ns" },
|
||||
};
|
||||
const next = applyProbeResult(typed, probeWithDefaults);
|
||||
assert.deepEqual(next.providerConfig, {
|
||||
context: "prod-us-west",
|
||||
namespace: "my-ns",
|
||||
inactivity_seconds: "1800",
|
||||
});
|
||||
});
|
||||
|
||||
test("probe resolution keeps a user-cleared field cleared", () => {
|
||||
// "" is a deliberate user state — coerceConfigValues drops empty numerics
|
||||
// and required-gating treats "" as incomplete; the probe must not undo it.
|
||||
const cleared = { ...unprobedDraft, providerConfig: { namespace: "" } };
|
||||
const next = applyProbeResult(cleared, probeWithDefaults);
|
||||
assert.equal(next.providerConfig.namespace, "");
|
||||
});
|
||||
|
||||
test("a schema-less probe result records the probe without touching config", () => {
|
||||
const typed = { ...unprobedDraft, providerConfig: { context: "abc" } };
|
||||
const next = applyProbeResult(typed, { ok: true });
|
||||
assert.deepEqual(next.providerConfig, { context: "abc" });
|
||||
assert.deepEqual(next.probedProvider, { ok: true });
|
||||
});
|
||||
|
||||
test("probe resolution preserves unrelated draft fields", () => {
|
||||
assert.equal(
|
||||
applyProbeResult(unprobedDraft, probeWithDefaults).runOn,
|
||||
"kubernetes",
|
||||
);
|
||||
});
|
||||
|
||||
@@ -15,6 +15,35 @@ export const emptyWhereToRunDraft: WhereToRunDraft = {
|
||||
probedProvider: null,
|
||||
};
|
||||
|
||||
/**
|
||||
* Fold a completed probe into the draft the user has *now* — not the draft
|
||||
* that existed when the probe started. Schema defaults prefill only the keys
|
||||
* the user has not touched: anything already in `providerConfig` (typed while
|
||||
* the probe was in flight) wins over the default. Overwriting instead of
|
||||
* merging is the "Typewriter Eraser" bug — every probe resolution silently
|
||||
* erased in-flight keystrokes.
|
||||
*/
|
||||
export function applyProbeResult(
|
||||
current: WhereToRunDraft,
|
||||
result: BackendProviderProbeResult,
|
||||
): WhereToRunDraft {
|
||||
const defaults: Record<string, string> = {};
|
||||
const properties =
|
||||
(result.config_schema as Record<string, unknown> | undefined)?.properties ??
|
||||
{};
|
||||
for (const [key, property] of Object.entries(properties) as [
|
||||
string,
|
||||
Record<string, unknown>,
|
||||
][]) {
|
||||
if (property.default != null) defaults[key] = String(property.default);
|
||||
}
|
||||
return {
|
||||
...current,
|
||||
probedProvider: result,
|
||||
providerConfig: { ...defaults, ...current.providerConfig },
|
||||
};
|
||||
}
|
||||
|
||||
export function providerConfigComplete(draft: WhereToRunDraft): boolean {
|
||||
if (draft.runOn === "local") return true;
|
||||
if (!draft.probedProvider) return false;
|
||||
|
||||
@@ -507,6 +507,11 @@ type E2eConfig = {
|
||||
* returning a catalog.
|
||||
*/
|
||||
discoverAgentModelsError?: string;
|
||||
// Backend provider mocks for the create-agent "Run on" section. See
|
||||
// tests/helpers/bridge.ts:MockBridgeOptions for semantics.
|
||||
backendProviders?: Array<{ id: string; binaryPath: string }>;
|
||||
backendProviderProbeResult?: Record<string, unknown>;
|
||||
backendProviderProbeDelayMs?: number;
|
||||
};
|
||||
relayHttpUrl?: string;
|
||||
relayWsUrl?: string;
|
||||
@@ -11064,9 +11069,22 @@ export function maybeInstallE2eTauriMocks() {
|
||||
activeConfig,
|
||||
);
|
||||
case "discover_backend_providers":
|
||||
return [];
|
||||
case "probe_backend_provider":
|
||||
return { ok: false, error: "mock: no providers available" };
|
||||
return activeConfig?.mock?.backendProviders ?? [];
|
||||
case "probe_backend_provider": {
|
||||
const probeDelayMs =
|
||||
activeConfig?.mock?.backendProviderProbeDelayMs ?? 0;
|
||||
if (probeDelayMs > 0) {
|
||||
await new Promise((resolve) =>
|
||||
window.setTimeout(resolve, probeDelayMs),
|
||||
);
|
||||
}
|
||||
return (
|
||||
activeConfig?.mock?.backendProviderProbeResult ?? {
|
||||
ok: false,
|
||||
error: "mock: no providers available",
|
||||
}
|
||||
);
|
||||
}
|
||||
case "discover_managed_agent_prereqs":
|
||||
return handleDiscoverManagedAgentPrereqs(
|
||||
payload as Parameters<typeof handleDiscoverManagedAgentPrereqs>[0],
|
||||
|
||||
@@ -0,0 +1,154 @@
|
||||
/**
|
||||
* E2E spec for the create-agent "Run on" provider config fields.
|
||||
*
|
||||
* Pins the fix for the "Typewriter Eraser": WhereToRunSection's probe effect
|
||||
* used to depend on the whole draft, so every keystroke re-probed the
|
||||
* provider and every probe resolution reset providerConfig to schema
|
||||
* defaults — typing into a defaultless field (the k8s "Kubeconfig context")
|
||||
* looked completely dead, and the provider binary respawned in a loop.
|
||||
*
|
||||
* Covers:
|
||||
* - typing into a defaultless provider field sticks, and the provider is
|
||||
* probed exactly once for the selection (not once per keystroke)
|
||||
* - the config form is gated on probe resolution (no half-rendered form),
|
||||
* and defaults prefill exactly once when a slow probe lands
|
||||
* - switching provider → local → provider re-probes and resets cleanly
|
||||
*
|
||||
* The stale-closure merge on probe resolution (defaults beneath in-flight
|
||||
* typing) is unreachable through this UI because the fields render only
|
||||
* after the probe resolves; it is pinned at the unit level in
|
||||
* whereToRunIntent.test.mjs (applyProbeResult).
|
||||
*/
|
||||
import { expect, test } from "@playwright/test";
|
||||
|
||||
import { installMockBridge } from "../helpers/bridge";
|
||||
|
||||
type Page = import("@playwright/test").Page;
|
||||
|
||||
const PROVIDER = {
|
||||
id: "kubernetes",
|
||||
binaryPath: "/mock/buzz-backend-kubernetes",
|
||||
};
|
||||
|
||||
const PROBE_RESULT = {
|
||||
ok: true,
|
||||
name: "kubernetes",
|
||||
version: "0.0.0-mock",
|
||||
config_schema: {
|
||||
type: "object",
|
||||
properties: {
|
||||
context: {
|
||||
type: "string",
|
||||
title: "Kubeconfig context",
|
||||
description: "Context from your kubeconfig.",
|
||||
},
|
||||
namespace: {
|
||||
type: "string",
|
||||
title: "Namespace",
|
||||
default: "buzz-agents-mock01",
|
||||
},
|
||||
},
|
||||
required: ["namespace"],
|
||||
},
|
||||
};
|
||||
|
||||
async function probeInvocations(page: Page): Promise<number> {
|
||||
return page.evaluate(
|
||||
() =>
|
||||
(
|
||||
window as Window & { __BUZZ_E2E_COMMANDS__?: string[] }
|
||||
).__BUZZ_E2E_COMMANDS__?.filter(
|
||||
(command) => command === "probe_backend_provider",
|
||||
).length ?? 0,
|
||||
);
|
||||
}
|
||||
|
||||
/** Open the create-agent dialog and select the mocked provider in "Run on". */
|
||||
async function openCreateDialogOnProvider(page: Page) {
|
||||
await page.goto("/", { waitUntil: "domcontentloaded" });
|
||||
await page.getByTestId("open-agents-view").click();
|
||||
await page.getByTestId("new-agent-card").click();
|
||||
await page.getByRole("menuitem", { name: "Create agent" }).click();
|
||||
const dialog = page.getByTestId("persona-dialog");
|
||||
await expect(dialog).toBeVisible({ timeout: 10_000 });
|
||||
await dialog.locator("#agent-run-on").selectOption(PROVIDER.id);
|
||||
return dialog;
|
||||
}
|
||||
|
||||
test("typing into a defaultless provider field sticks and probes only once", async ({
|
||||
page,
|
||||
}) => {
|
||||
await installMockBridge(page, {
|
||||
backendProviders: [PROVIDER],
|
||||
backendProviderProbeResult: PROBE_RESULT,
|
||||
});
|
||||
const dialog = await openCreateDialogOnProvider(page);
|
||||
|
||||
const contextField = dialog.locator("#provider-cfg-context");
|
||||
await expect(contextField).toBeVisible({ timeout: 10_000 });
|
||||
// Defaults prefilled from the schema; context has none.
|
||||
await expect(dialog.locator("#provider-cfg-namespace")).toHaveValue(
|
||||
"buzz-agents-mock01",
|
||||
);
|
||||
await expect(contextField).toHaveValue("");
|
||||
|
||||
await contextField.pressSequentially("prod-us-west", { delay: 20 });
|
||||
await expect(contextField).toHaveValue("prod-us-west");
|
||||
|
||||
// One selection, one probe — keystrokes must not refire it.
|
||||
expect(await probeInvocations(page)).toBe(1);
|
||||
});
|
||||
|
||||
test("config fields render only after a slow probe resolves, with defaults", async ({
|
||||
page,
|
||||
}) => {
|
||||
// The fields are gated on the probe result (draft.probedProvider), which is
|
||||
// what makes mid-flight typing unreachable through the UI — the stale-probe
|
||||
// merge seam (applyProbeResult) is pinned at the unit level instead. This
|
||||
// spec holds the gate: no half-rendered form before the probe lands, and
|
||||
// defaults appear exactly once when it does.
|
||||
await installMockBridge(page, {
|
||||
backendProviders: [PROVIDER],
|
||||
backendProviderProbeResult: PROBE_RESULT,
|
||||
backendProviderProbeDelayMs: 1_000,
|
||||
});
|
||||
const dialog = await openCreateDialogOnProvider(page);
|
||||
|
||||
// Pre-resolution: the security warning is up, the form is not.
|
||||
await expect(dialog.getByText("will receive your agent")).toBeVisible();
|
||||
await expect(dialog.locator("#provider-cfg-context")).toHaveCount(0);
|
||||
|
||||
// Post-resolution: fields render with schema defaults prefilled.
|
||||
await expect(dialog.locator("#provider-cfg-context")).toBeVisible({
|
||||
timeout: 10_000,
|
||||
});
|
||||
await expect(dialog.locator("#provider-cfg-namespace")).toHaveValue(
|
||||
"buzz-agents-mock01",
|
||||
);
|
||||
expect(await probeInvocations(page)).toBe(1);
|
||||
});
|
||||
|
||||
test("provider → local → provider re-probes and resets the config", async ({
|
||||
page,
|
||||
}) => {
|
||||
await installMockBridge(page, {
|
||||
backendProviders: [PROVIDER],
|
||||
backendProviderProbeResult: PROBE_RESULT,
|
||||
});
|
||||
const dialog = await openCreateDialogOnProvider(page);
|
||||
|
||||
const contextField = dialog.locator("#provider-cfg-context");
|
||||
await expect(contextField).toBeVisible({ timeout: 10_000 });
|
||||
await contextField.fill("stale-value");
|
||||
|
||||
await dialog.locator("#agent-run-on").selectOption("local");
|
||||
await expect(contextField).toHaveCount(0);
|
||||
|
||||
await dialog.locator("#agent-run-on").selectOption(PROVIDER.id);
|
||||
await expect(dialog.locator("#provider-cfg-context")).toBeVisible({
|
||||
timeout: 10_000,
|
||||
});
|
||||
// Fresh selection = fresh draft: the stale value must not leak back.
|
||||
await expect(dialog.locator("#provider-cfg-context")).toHaveValue("");
|
||||
expect(await probeInvocations(page)).toBe(2);
|
||||
});
|
||||
@@ -524,6 +524,25 @@ type MockBridgeOptions = {
|
||||
* returning a catalog. Exercises the discovery-failure UI path.
|
||||
*/
|
||||
discoverAgentModelsError?: string;
|
||||
/**
|
||||
* Providers returned by `discover_backend_providers`. Defaults to `[]`
|
||||
* (the "Run on" section stays hidden). Setting this renders the remote
|
||||
* backend selector in the create-agent dialog.
|
||||
*/
|
||||
backendProviders?: Array<{ id: string; binaryPath: string }>;
|
||||
/**
|
||||
* Result returned by `probe_backend_provider`. Defaults to
|
||||
* `{ ok: false, error: "mock: no providers available" }`.
|
||||
*/
|
||||
backendProviderProbeResult?: Record<string, unknown>;
|
||||
/**
|
||||
* Delay (ms) applied to `probe_backend_provider` so a spec can assert the
|
||||
* pre-resolution state (config fields stay probe-gated until the result
|
||||
* lands). Typing while a probe is in flight is unreachable through the UI
|
||||
* for the same reason; that merge path is pinned at the unit level
|
||||
* (`applyProbeResult` in whereToRunIntent.test.mjs).
|
||||
*/
|
||||
backendProviderProbeDelayMs?: number;
|
||||
};
|
||||
|
||||
type BridgeOptions = {
|
||||
|
||||
Reference in New Issue
Block a user