Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ export function ToolsTab({
searchQuery = "",
}) {
const t = useT();
const { tools, query, setPermission, savedTools } = useTools();
const { tools, query, setPermission, savedTools, error: permissionError } = useTools();
Comment thread
italic-jinxin marked this conversation as resolved.

if (query.isLoading) {
return html`
Expand Down Expand Up @@ -253,6 +253,16 @@ export function ToolsTab({
isLoading=${isLoading}
/>

${permissionError &&
html`
<div
className="rounded-md border border-[color-mix(in_srgb,var(--v2-danger-text)_30%,transparent)] bg-[var(--v2-danger-soft)] px-4 py-3 text-sm text-[var(--v2-danger-text)]"
role="alert"
>
${t("error.saveFailed", { message: permissionError.message })}
</div>
`}

${searchQuery &&
html`
<div className="flex justify-end">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,21 +74,29 @@ function componentProps(node, component) {
return props;
}

function renderToolsModule({ tools = [], translations = {} } = {}) {
function renderToolsModule({ tools = [], translations = {}, toolError = null } = {}) {
const saved = [];
const translate = (key, params = {}) => {
let value = translations[key] || key;
for (const [name, replacement] of Object.entries(params)) {
value = value.replaceAll(`{${name}}`, String(replacement));
}
return value;
};
const context = {
Badge: "Badge",
Card: "Card",
Icon: "Icon",
html,
matchesSearch: (query, values) =>
!query || values.some((value) => String(value || "").includes(query)),
useT: () => (key) => translations[key] || key,
useT: () => translate,
useTools: () => ({
tools,
query: { isLoading: false, error: null },
setPermission: () => {},
savedTools: {},
error: toolError,
}),
};
const exports = runVmModuleForTest(
Expand Down Expand Up @@ -295,3 +303,19 @@ test("Tools tab search does not index locked tools as disabled unless disabled",
assert.equal(findComponentNode(rendered, exports.ToolRow), null);
assert.ok(collectScalars(rendered).includes("tools.noMatch"));
});

test("Tools tab surfaces permission save failures", () => {
const { exports } = renderToolsModule({
translations: {
"error.saveFailed": "Save failed: {message}",
},
toolError: new Error("permission denied"),
});

const rendered = exports.ToolsTab({});

assert.match(collectTemplateText(rendered), /role="alert"/);
assert.match(collectTemplateText(rendered), /var\(--v2-danger-text\)/);
assert.doesNotMatch(collectTemplateText(rendered), /text-red-|bg-red-|border-red-/);
assert.ok(collectScalars(rendered).includes("Save failed: permission denied"));
});
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,10 @@ export function useTools() {
});

const setPermission = React.useCallback(
(name, state) => mutation.mutate({ name, state }),
(name, state) => {
mutation.reset();
mutation.mutate({ name, state });
},
[mutation]
);

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import assert from "node:assert/strict";
import test from "node:test";

import { runVmModuleForTest } from "../../../test-support/vm-module-harness.test.mjs";

function loadUseTools({ mutationError = null } = {}) {
const calls = [];
const context = {
React: {
useCallback: (fn) => fn,
useState: (initial) => [
initial,
(updater) => calls.push({ type: "setState", updater }),
],
},
fetchTools: () => {},
globalThis: {},
throwIfApiFailed: (value) => value,
updateToolPermission: () => {},
useMutation: (options) => ({
error: mutationError,
mutate: (payload) => calls.push({ type: "mutate", payload }),
reset: () => calls.push({ type: "reset" }),
options,
}),
useQuery: () => ({ data: { tools: [] }, isLoading: false, error: null }),
useQueryClient: () => ({
setQueryData: (...args) => calls.push({ type: "setQueryData", args }),
}),
};

const exports = runVmModuleForTest("./useTools.js", ["useTools"], context, import.meta.url);

return {
calls,
useTools: exports.useTools,
};
}

test("setPermission clears the previous mutation error before retrying save", () => {
const { calls, useTools } = loadUseTools({ mutationError: new Error("old failure") });
const tools = useTools();

tools.setPermission("builtin.echo", "disabled");

assert.equal(calls[0].type, "reset");
assert.equal(calls[1].type, "mutate");
assert.equal(calls[1].payload.name, "builtin.echo");
assert.equal(calls[1].payload.state, "disabled");
});
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,9 @@ def _tool_entry(
}


async def _open_mocked_tools_page(reborn_v2_server, reborn_v2_browser):
async def _open_mocked_tools_page(
reborn_v2_server, reborn_v2_browser, *, fail_permission_saves: bool = False
):
context = await reborn_v2_browser.new_context(viewport={"width": 1280, "height": 720})
page = await context.new_page()
auto_approve = {"enabled": False}
Expand Down Expand Up @@ -130,6 +132,14 @@ async def handle_settings_tools(route):
name = unquote(path.removeprefix("/api/webchat/v2/settings/tools/"))
body = json.loads(request.post_data or "{}")
permission_requests.append({"name": name, "body": body})
if fail_permission_saves:
await fulfill_json(
route,
{"kind": "permission_denied", "error": "permission_denied"},
status=403,
)
return

tool = tool_states[name]
if not tool["mutable"]:
await fulfill_json(
Expand Down Expand Up @@ -300,6 +310,34 @@ async def test_reborn_legacy_tool_permission_select_persists_after_reload(
await harness["context"].close()


async def test_reborn_legacy_tool_permission_save_failure_shows_error(
reborn_v2_server, reborn_v2_browser
):
harness = await _open_mocked_tools_page(
reborn_v2_server,
reborn_v2_browser,
fail_permission_saves=True,
)
try:
page = harness["page"]
select = page.get_by_label("Permission for echo")
await expect(select).to_have_value("always_allow", timeout=5000)

await select.select_option("ask_each_time")

await expect(page.get_by_role("alert")).to_contain_text(
"Save failed: Permission denied",
timeout=5000,
)
await expect(select).to_have_value("always_allow", timeout=5000)
assert harness["permission_requests"][-1] == {
"name": "echo",
"body": {"state": "ask_each_time"},
}
finally:
await harness["context"].close()


async def test_reborn_legacy_locked_tool_shows_badge_without_select(
reborn_v2_server, reborn_v2_browser
):
Expand Down
Loading