mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-28 13:26:04 -06:00
fix(tui): dismiss 'loading models...' notice once the model picker opens (#129770)
* fix(tui): dismiss 'loading models...' notice once the model picker opens The model selector appended a permanent 'loading models...' system message before awaiting client.listModels(). Unlike other transient notices, it was never removed once the picker rendered, leaving a stale line in the chat log even after the model was selected. Switch to the existing addPendingSystem/dismissPendingSystem mechanism (keyed by a stable 'model-selector' run id) so the notice is cleared when the list resolves. The empty-list and error branches are unchanged, and a stale request (session switch) still skips dismissal since the notice is tied to the current picker request. Regression tests: - picker opens => addPendingSystem called, then dismissPendingSystem - empty list => dismiss + 'no models available' - list failure => no dismiss, 'model list failed: <err>' - stale session switch => no dismiss, no overlay Fixes #129756 * fix(tui): also dismiss 'loading models...' on the error path Copilot review (#129770) caught that the error branch left the pending notice visible after appending the terminal 'model list failed: ...' line. Dismiss it before the terminal notice so the user sees exactly one system message for the failed listing. Also update the regression test to assert dismissal on the error path. * fix(tui): retire model picker notices with their requests Co-authored-by: lazytalk <lazytalk@users.noreply.github.com> * fix(tui): narrow picker notice regression test ids Co-authored-by: lazytalk <lazytalk@users.noreply.github.com> --------- Co-authored-by: lazytalk <lazytalk@users.noreply.github.com> Co-authored-by: Peter Steinberger <steipete@gmail.com>
This commit is contained in:
@@ -161,6 +161,11 @@ function createHarness(params?: {
|
||||
const dropPendingUser = vi.fn();
|
||||
const rekeyPendingUser = vi.fn();
|
||||
const addSystem = vi.fn();
|
||||
const pendingSystemNotices = new Map<string, string>();
|
||||
const addPendingSystem = vi.fn((runId: string, text: string) => {
|
||||
pendingSystemNotices.set(runId, text);
|
||||
});
|
||||
const dismissPendingSystem = vi.fn((runId: string) => pendingSystemNotices.delete(runId));
|
||||
const clearTools = vi.fn();
|
||||
const reserveAssistantSlot = vi.fn();
|
||||
const requestRender = vi.fn();
|
||||
@@ -228,6 +233,8 @@ function createHarness(params?: {
|
||||
dropPendingUser,
|
||||
rekeyPendingUser,
|
||||
addSystem,
|
||||
addPendingSystem,
|
||||
dismissPendingSystem,
|
||||
clearTools,
|
||||
reserveAssistantSlot,
|
||||
} as never,
|
||||
@@ -282,6 +289,9 @@ function createHarness(params?: {
|
||||
dropPendingUser,
|
||||
rekeyPendingUser,
|
||||
addSystem,
|
||||
addPendingSystem,
|
||||
dismissPendingSystem,
|
||||
pendingSystemNotices,
|
||||
clearTools,
|
||||
reserveAssistantSlot,
|
||||
requestRender,
|
||||
@@ -363,7 +373,14 @@ describe("tui command handlers", () => {
|
||||
});
|
||||
|
||||
const olderPicker = harness.handleCommand("/models");
|
||||
expect(harness.pendingSystemNotices.size).toBe(1);
|
||||
const olderNoticeId = expectDefined(
|
||||
harness.pendingSystemNotices.keys().next().value,
|
||||
"older model picker notice",
|
||||
);
|
||||
await harness.handleCommand(newerCommand);
|
||||
expect(harness.pendingSystemNotices.has(olderNoticeId)).toBe(false);
|
||||
expect(harness.dismissPendingSystem).toHaveBeenCalledWith(olderNoticeId);
|
||||
olderModels.resolve([{ provider: "openai", id: "obsolete-model" }]);
|
||||
await olderPicker;
|
||||
|
||||
@@ -3180,15 +3197,17 @@ describe("tui command handlers", () => {
|
||||
},
|
||||
);
|
||||
const listModels = vi.fn(() => listModelsPromise);
|
||||
const { handleCommand, addSystem, openOverlay, requestRender } = createHarness({ listModels });
|
||||
const { handleCommand, addPendingSystem, openOverlay, requestRender } = createHarness({
|
||||
listModels,
|
||||
});
|
||||
|
||||
const pending = handleCommand("/models");
|
||||
await Promise.resolve();
|
||||
|
||||
expect(listModels).toHaveBeenCalledTimes(1);
|
||||
expect(addSystem).toHaveBeenCalledWith("loading models...");
|
||||
expect(addPendingSystem).toHaveBeenCalledWith(expect.any(String), "loading models...");
|
||||
expect(openOverlay).not.toHaveBeenCalled();
|
||||
const feedbackOrder = addSystem.mock.invocationCallOrder[0] ?? 0;
|
||||
const feedbackOrder = addPendingSystem.mock.invocationCallOrder[0] ?? 0;
|
||||
const renderOrders = requestRender.mock.invocationCallOrder;
|
||||
expect(renderOrders.filter((order) => order > feedbackOrder)).not.toEqual([]);
|
||||
|
||||
@@ -3198,6 +3217,70 @@ describe("tui command handlers", () => {
|
||||
expect(openOverlay).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it.each([
|
||||
{
|
||||
name: "successful listing",
|
||||
listModels: vi.fn().mockResolvedValue([{ provider: "openai", id: "picker-model" }]),
|
||||
terminalNotice: undefined,
|
||||
},
|
||||
{
|
||||
name: "empty listing",
|
||||
listModels: vi.fn().mockResolvedValue([]),
|
||||
terminalNotice: "no models available",
|
||||
},
|
||||
{
|
||||
name: "failed listing",
|
||||
listModels: vi.fn().mockRejectedValue(new Error("fixture backend unavailable")),
|
||||
terminalNotice: "model list failed: fixture backend unavailable",
|
||||
},
|
||||
])("removes temporary model feedback after $name", async ({ listModels, terminalNotice }) => {
|
||||
const harness = createHarness({ listModels });
|
||||
|
||||
await harness.handleCommand("/models");
|
||||
|
||||
expect(harness.addPendingSystem).toHaveBeenCalledWith(expect.any(String), "loading models...");
|
||||
expect(harness.pendingSystemNotices.size).toBe(0);
|
||||
expect(harness.dismissPendingSystem).toHaveBeenCalledOnce();
|
||||
if (terminalNotice) {
|
||||
expect(harness.addSystem).toHaveBeenCalledWith(terminalNotice);
|
||||
}
|
||||
});
|
||||
|
||||
it("does not let an older model request remove the newer request's notice", async () => {
|
||||
const olderModels = createDeferred<Array<{ provider: string; id: string }>>();
|
||||
const newerModels = createDeferred<Array<{ provider: string; id: string }>>();
|
||||
const harness = createHarness({
|
||||
listModels: vi
|
||||
.fn()
|
||||
.mockReturnValueOnce(olderModels.promise)
|
||||
.mockReturnValueOnce(newerModels.promise),
|
||||
});
|
||||
|
||||
const olderPicker = harness.handleCommand("/models");
|
||||
const olderNoticeId = expectDefined(
|
||||
harness.pendingSystemNotices.keys().next().value,
|
||||
"older model picker notice",
|
||||
);
|
||||
const newerPicker = harness.handleCommand("/models");
|
||||
const newerNoticeId = expectDefined(
|
||||
harness.pendingSystemNotices.keys().next().value,
|
||||
"newer model picker notice",
|
||||
);
|
||||
|
||||
expect(newerNoticeId).not.toBe(olderNoticeId);
|
||||
expect(harness.pendingSystemNotices.has(olderNoticeId)).toBe(false);
|
||||
expect(harness.pendingSystemNotices.has(newerNoticeId)).toBe(true);
|
||||
|
||||
olderModels.resolve([{ provider: "openai", id: "obsolete-model" }]);
|
||||
await olderPicker;
|
||||
expect(harness.pendingSystemNotices.has(newerNoticeId)).toBe(true);
|
||||
|
||||
newerModels.resolve([{ provider: "openai", id: "current-model" }]);
|
||||
await newerPicker;
|
||||
expect(harness.pendingSystemNotices.size).toBe(0);
|
||||
expect(harness.openOverlay).toHaveBeenCalledOnce();
|
||||
});
|
||||
|
||||
it("does not open a stale model selector after switching sessions", async () => {
|
||||
const deferred = createDeferred<Array<{ provider: string; id: string; name?: string }>>();
|
||||
const harness = createHarness({
|
||||
@@ -3206,14 +3289,14 @@ describe("tui command handlers", () => {
|
||||
});
|
||||
|
||||
const pending = harness.handleCommand("/models");
|
||||
expect(harness.addSystem).toHaveBeenCalledWith("loading models...");
|
||||
harness.addSystem.mockClear();
|
||||
expect(harness.addPendingSystem).toHaveBeenCalledWith(expect.any(String), "loading models...");
|
||||
harness.state.currentSessionKey = "agent:main:second";
|
||||
deferred.resolve([{ provider: "openai", id: "gpt-5.6-luna" }]);
|
||||
await pending;
|
||||
|
||||
expect(harness.openOverlay).not.toHaveBeenCalled();
|
||||
expect(harness.addSystem).not.toHaveBeenCalled();
|
||||
expect(harness.pendingSystemNotices.size).toBe(0);
|
||||
});
|
||||
|
||||
it.each([
|
||||
|
||||
@@ -152,7 +152,7 @@ export function createCommandHandlers(context: CommandHandlerContext) {
|
||||
boundary: null as "new" | "reset" | null,
|
||||
epoch: 0,
|
||||
};
|
||||
let pickerRequest: { overlay?: OverlayHandle } | null = null;
|
||||
let pickerRequest: { overlay?: OverlayHandle; noticeId: string } | null = null;
|
||||
|
||||
// Hold one owner through the full identity transition so later input cannot
|
||||
// target the session being retired while create/reset awaits the backend.
|
||||
@@ -223,11 +223,14 @@ export function createCommandHandlers(context: CommandHandlerContext) {
|
||||
chatLog.addSystem(`agent set to ${state.currentAgentId}; use /openclaw to return`);
|
||||
};
|
||||
|
||||
const beginPickerRequest = (): { overlay?: OverlayHandle } => {
|
||||
const beginPickerRequest = (): { overlay?: OverlayHandle; noticeId: string } => {
|
||||
if (pickerRequest && chatLog.dismissPendingSystem(pickerRequest.noticeId)) {
|
||||
tui.requestRender();
|
||||
}
|
||||
if (pickerRequest?.overlay) {
|
||||
closeOverlayAndRender(pickerRequest.overlay);
|
||||
}
|
||||
return (pickerRequest = {});
|
||||
return (pickerRequest = { noticeId: randomUUID() });
|
||||
};
|
||||
|
||||
const closeOverlayAndRender = (handle: OverlayHandle) => {
|
||||
@@ -349,7 +352,7 @@ export function createCommandHandlers(context: CommandHandlerContext) {
|
||||
const request = beginPickerRequest();
|
||||
const selection = captureSessionSelection();
|
||||
try {
|
||||
chatLog.addSystem("loading models...");
|
||||
chatLog.addPendingSystem(request.noticeId, "loading models...");
|
||||
tui.requestRender();
|
||||
const models = await client.listModels({ agentId: selection.agentId });
|
||||
if (request !== pickerRequest || !isCurrentSessionSelection(selection)) {
|
||||
@@ -357,7 +360,6 @@ export function createCommandHandlers(context: CommandHandlerContext) {
|
||||
}
|
||||
if (models.length === 0) {
|
||||
chatLog.addSystem("no models available");
|
||||
tui.requestRender();
|
||||
return;
|
||||
}
|
||||
const items = models.map((model) => {
|
||||
@@ -379,7 +381,10 @@ export function createCommandHandlers(context: CommandHandlerContext) {
|
||||
return;
|
||||
}
|
||||
chatLog.addSystem(`model list failed: ${formatTuiErrorMessage(err)}`);
|
||||
tui.requestRender();
|
||||
} finally {
|
||||
if (request === pickerRequest && chatLog.dismissPendingSystem(request.noticeId)) {
|
||||
tui.requestRender();
|
||||
}
|
||||
}
|
||||
};
|
||||
|
||||
|
||||
@@ -47,7 +47,8 @@ export async function exerciseTuiCommandSurface(
|
||||
if (surface === "pickers") {
|
||||
await fixture.run.write("/models\r", { delay: false });
|
||||
await fixture.waitForLogEntry((entry) => entry.method === "listModels");
|
||||
await waitForRows((rows) => rows.some((row) => row.includes("Fixture 2")));
|
||||
const pickerRows = await waitForRows((rows) => rows.some((row) => row.includes("Fixture 2")));
|
||||
expect(pickerRows.some((row) => row.includes("loading models..."))).toBe(false);
|
||||
await fixture.run.write("\x1b[B\r", { delay: false });
|
||||
await fixture.waitForLogEntry(
|
||||
(entry) =>
|
||||
|
||||
Reference in New Issue
Block a user