fix(custom-widgets): address review findings
This commit is contained in:
1 parent
349f889464
commit
6cca2bafe6
12 files changed
+158
-21
No files matched your search
@@ -8,7 +8,7 @@ Templates read `data.requestId`, `status.requestId`, `options.name`, and tempora
|
||||
<TextInput bind="search" label="Search" />
|
||||
<Pagination bind="page" resetKey={inputs.search} defaultValue={1} total={5} />
|
||||
<SubFetch requestId="search" trigger="manual" params={{ query: inputs.search }}>
|
||||
{(items) => <Stack>{(items ?? []).map(item => <Text key={item.id}>{item.name}</Text>)}</Stack>}
|
||||
{(result) => <Stack>{(result.results ?? []).map(item => <Text key={item.id}>{item.name}</Text>)}</Stack>}
|
||||
</SubFetch>
|
||||
```
|
||||
|
||||
|
||||
@@ -15,6 +15,15 @@ const userMessage = (text: string): UIMessage => ({
|
||||
});
|
||||
|
||||
describe("Custom Widget authoring context", () => {
|
||||
test("refunds a focused discovery claim when the tool call fails", () => {
|
||||
const controller = createCustomWidgetDiscoveryPhaseController();
|
||||
|
||||
expect(controller.claim("customWidget_getComponents")).toBe(true);
|
||||
expect(controller.claim("customWidget_getComponents")).toBe(false);
|
||||
controller.observeFailure("customWidget_getComponents");
|
||||
expect(controller.claim("customWidget_getComponents")).toBe(true);
|
||||
});
|
||||
|
||||
test.each([
|
||||
"Create a custom widget for these fixtures",
|
||||
"Create custom widgets for these services",
|
||||
|
||||
@@ -42,7 +42,17 @@ export const createCustomWidgetDiscoveryPhaseController = (limit = maxFocusedCom
|
||||
if (toolName === "customWidget_createFromPreview" && typeof result?.id === "string") reset();
|
||||
},
|
||||
observeFailure(toolName: string) {
|
||||
if (toolName === "customWidget_validateTemplate" || toolName === "customWidget_previewCreate") reset();
|
||||
if (toolName === "customWidget_validateTemplate" || toolName === "customWidget_previewCreate") {
|
||||
reset();
|
||||
return;
|
||||
}
|
||||
if (customWidgetContextToolBudgets[toolName] === undefined) return;
|
||||
const used = calls.get(toolName) ?? 0;
|
||||
if (used <= 1) {
|
||||
calls.delete(toolName);
|
||||
return;
|
||||
}
|
||||
calls.set(toolName, used - 1);
|
||||
},
|
||||
};
|
||||
};
|
||||
|
||||
@@ -11,7 +11,7 @@ import {
|
||||
import { permissionRequiredProcedure } from "../../trpc";
|
||||
import { parseCustomWidgetAuthoringInput } from "./authoring-validation";
|
||||
import { insertCustomWidgetDefinition } from "./definition-insert";
|
||||
import { getPreviewJournal, getPreviewSession, getPreviewSessionSecrets } from "./preview-sessions";
|
||||
import { getPreviewEvidence, getPreviewSession, getPreviewSessionSecrets } from "./preview-sessions";
|
||||
import { assertSecretSources } from "./secret-policy";
|
||||
|
||||
const logger = createLogger({ module: "custom-widget" });
|
||||
@@ -37,7 +37,7 @@ export const creationProcedures = {
|
||||
mcp: {
|
||||
enabled: true,
|
||||
description:
|
||||
"Create a Custom JSX widget without reusing a preview. Prefer customWidget_createFromPreview after full preview validation and query checks so the definition is not streamed again.",
|
||||
"Requires administrator permission. Create a Custom JSX widget without reusing a preview. Prefer customWidget_createFromPreview after full preview validation and query checks so the definition is not streamed again.",
|
||||
},
|
||||
})
|
||||
.input(customWidgetCreateSchema)
|
||||
@@ -56,16 +56,16 @@ export const creationProcedures = {
|
||||
mcp: {
|
||||
enabled: true,
|
||||
description:
|
||||
"Persist the exact tested definition from a customWidget_previewCreate or customWidget_previewReviseTemplate session. Prefer this over resending a large widget to customWidget_create. Every query and action in the final preview revision must have current evidence.",
|
||||
"Requires administrator permission. Persist the exact tested definition from a customWidget_previewCreate or customWidget_previewReviseTemplate session. Prefer this over resending a large widget to customWidget_create. Every query and action in the final preview revision must have current evidence.",
|
||||
},
|
||||
})
|
||||
.input(z.object({ previewSessionId: z.string().min(1), targetBoardId: z.string().min(1).optional() }))
|
||||
.mutation(async ({ ctx, input }) => {
|
||||
const session = await getPreviewSession(input.previewSessionId, ctx.session.user.id);
|
||||
const journal = await getPreviewJournal(session.id, ctx.session.user.id);
|
||||
const evidence = await getPreviewEvidence(session.id, ctx.session.user.id);
|
||||
const unverifiedQueryIds = Object.entries(session.requests).flatMap(([requestId, request]) => {
|
||||
if (request.kind !== "query") return [];
|
||||
const verified = journal.some(
|
||||
const verified = evidence.some(
|
||||
(entry) =>
|
||||
entry.kind === "query" &&
|
||||
entry.requestId === requestId &&
|
||||
@@ -84,12 +84,8 @@ export const creationProcedures = {
|
||||
}
|
||||
const unverifiedActionIds = Object.entries(session.requests).flatMap(([requestId, request]) => {
|
||||
if (request.kind !== "action") return [];
|
||||
const verified = journal.some((entry) => {
|
||||
if (
|
||||
entry.kind !== "action" ||
|
||||
entry.requestId !== requestId ||
|
||||
entry.sessionRevision !== session.revision
|
||||
) {
|
||||
const verified = evidence.some((entry) => {
|
||||
if (entry.kind !== "action" || entry.requestId !== requestId || entry.sessionRevision !== session.revision) {
|
||||
return false;
|
||||
}
|
||||
if (entry.simulated) return true;
|
||||
|
||||
@@ -14,6 +14,7 @@ import { toTrpcError } from "./domain-error";
|
||||
|
||||
const SESSION_PREFIX = "custom-widget:preview-session:";
|
||||
const JOURNAL_PREFIX = "custom-widget:preview-journal:";
|
||||
const EVIDENCE_PREFIX = "custom-widget:preview-evidence:";
|
||||
const COMPARE_AND_SWAP_SESSION_SCRIPT = `
|
||||
local current = redis.call('GET', KEYS[1])
|
||||
if not current then return 0 end
|
||||
@@ -64,6 +65,13 @@ class RedisPreviewSessionStore implements PreviewSessionStore {
|
||||
public getJournal(id: string, maxEntries: number) {
|
||||
return this.redis.lrange(`${JOURNAL_PREFIX}${id}`, 0, maxEntries - 1);
|
||||
}
|
||||
public async saveEvidence(id: string, key: string, value: unknown, ttlMs: number) {
|
||||
const evidenceKey = `${EVIDENCE_PREFIX}${id}`;
|
||||
await this.redis.multi().hset(evidenceKey, key, JSON.stringify(value)).pexpire(evidenceKey, ttlMs).exec();
|
||||
}
|
||||
public getEvidence(id: string) {
|
||||
return this.redis.hvals(`${EVIDENCE_PREFIX}${id}`);
|
||||
}
|
||||
}
|
||||
|
||||
let service: CustomWidgetPreviewSessionService | undefined;
|
||||
@@ -104,6 +112,7 @@ export const appendPreviewJournal = (
|
||||
entry: Omit<CustomWidgetPreviewJournalEntry, "id" | "sessionRevision" | "timestamp">,
|
||||
) => call(() => getService().appendJournal(session, { ...entry, sessionRevision: session.revision }));
|
||||
export const getPreviewJournal = (id: string, userId: string) => call(() => getService().getJournal(id, userId));
|
||||
export const getPreviewEvidence = (id: string, userId: string) => call(() => getService().getEvidence(id, userId));
|
||||
export const getPreviewSessionSecrets = (session: CustomWidgetPreviewSession, sourceId: string) =>
|
||||
getService().getSecrets(session, sourceId);
|
||||
export const setPreviewSessionSecrets = (id: string, userId: string, secrets: CreatePreviewSessionInput["secrets"]) =>
|
||||
|
||||
@@ -18,6 +18,7 @@ import { configureCustomWidgetSourceFromRequest } from "../../custom-widget/secr
|
||||
import {
|
||||
appendPreviewJournal,
|
||||
configurePreviewSessionSource,
|
||||
getPreviewJournal,
|
||||
getPreviewSession,
|
||||
} from "../../custom-widget/preview-sessions";
|
||||
import { serializeCustomWidgetDefinition } from "../../custom-widget/stored-definition";
|
||||
@@ -294,6 +295,43 @@ describe("custom widget definition persistence", () => {
|
||||
await expect(caller.get({ id: created.id })).resolves.toMatchObject({ template: revisedTemplate });
|
||||
});
|
||||
|
||||
test("retains persistence evidence for every request beyond the bounded journal", async () => {
|
||||
const db = await prepareDatabase();
|
||||
const caller = createCaller(db);
|
||||
const requests = Object.fromEntries(
|
||||
Array.from({ length: 64 }, (_, index) => [`query-${index}`, { path: `/queries/${index}` }]),
|
||||
);
|
||||
const definition = customWidgetDefinitionSchema.parse({
|
||||
$schema: "homarr-custom-widget-v2",
|
||||
name: "Large request set",
|
||||
sources: {
|
||||
default: { baseUrl: "https://example.com", networkScope: "public", auth: "none" },
|
||||
},
|
||||
requests,
|
||||
options: {},
|
||||
template: "<Text>All requests tested</Text>",
|
||||
});
|
||||
const preview = await caller.previewCreate({ definition, secrets: [] });
|
||||
const previewSession = await getPreviewSession(preview.previewSession.id, userId);
|
||||
|
||||
for (const [requestId, request] of Object.entries(previewSession.requests)) {
|
||||
await appendPreviewJournal(previewSession, {
|
||||
requestId,
|
||||
kind: "query",
|
||||
method: request.method,
|
||||
path: request.path,
|
||||
status: 200,
|
||||
durationMs: 1,
|
||||
simulated: false,
|
||||
});
|
||||
}
|
||||
|
||||
expect(await getPreviewJournal(previewSession.id, userId)).toHaveLength(50);
|
||||
await expect(caller.createFromPreview({ previewSessionId: previewSession.id })).resolves.toMatchObject({
|
||||
managementPath: expect.stringContaining("/manage/custom-widgets/edit/"),
|
||||
});
|
||||
});
|
||||
|
||||
test("updates a saved widget with MCP-friendly multiline templateLines", async () => {
|
||||
const db = await prepareDatabase();
|
||||
const caller = createCaller(db);
|
||||
|
||||
@@ -205,6 +205,17 @@ test("MCP tools are deterministically ordered and retain executable schemas", ()
|
||||
expect(tools.every((tool) => tool.inputValidator instanceof z.ZodObject)).toBe(true);
|
||||
});
|
||||
|
||||
test("publishes both Custom Widget template input formats", () => {
|
||||
const tool = extractMcpToolsFromProcedures(mcpRouter).find(
|
||||
(candidate) => candidate.name === "customWidget_validateTemplate",
|
||||
);
|
||||
|
||||
expect(tool?.inputSchema.properties).toMatchObject({
|
||||
template: expect.any(Object),
|
||||
templateLines: expect.any(Object),
|
||||
});
|
||||
});
|
||||
|
||||
describe("custom widget authoring procedure access", () => {
|
||||
const unauthenticatedCaller = mcpRouter.createCaller({
|
||||
db: null as never,
|
||||
|
||||
@@ -96,7 +96,7 @@ Templates read \`data.requestId\`, \`status.requestId\`, \`options.name\`, and t
|
||||
<TextInput bind="search" label="Search" />
|
||||
<Pagination bind="page" resetKey={inputs.search} defaultValue={1} total={5} />
|
||||
<SubFetch requestId="search" trigger="manual" params={{ query: inputs.search }}>
|
||||
{(items) => <Stack>{(items ?? []).map(item => <Text key={item.id}>{item.name}</Text>)}</Stack>}
|
||||
{(result) => <Stack>{(result.results ?? []).map(item => <Text key={item.id}>{item.name}</Text>)}</Stack>}
|
||||
</SubFetch>
|
||||
\`\`\`
|
||||
|
||||
|
||||
@@ -12,6 +12,22 @@ const methods = new Set(["GET", "POST", "PUT", "PATCH", "DELETE"]);
|
||||
const kinds = new Set(["query", "action"]);
|
||||
const permissions = new Set(["view", "modify", "full"]);
|
||||
|
||||
const parseConfirmation = (value: unknown): CustomJsxRequestCapability["confirmation"] => {
|
||||
if (!value || typeof value !== "object" || Array.isArray(value)) return undefined;
|
||||
const record = value as Record<string, unknown>;
|
||||
if (typeof record.title !== "string" || typeof record.message !== "string") return undefined;
|
||||
if (record.confirmLabel !== undefined && typeof record.confirmLabel !== "string") return undefined;
|
||||
if (record.destructive !== undefined && typeof record.destructive !== "boolean") return undefined;
|
||||
|
||||
const confirmation: NonNullable<CustomJsxRequestCapability["confirmation"]> = {
|
||||
title: record.title,
|
||||
message: record.message,
|
||||
};
|
||||
if (typeof record.confirmLabel === "string") confirmation.confirmLabel = record.confirmLabel;
|
||||
if (typeof record.destructive === "boolean") confirmation.destructive = record.destructive;
|
||||
return confirmation;
|
||||
};
|
||||
|
||||
export function parseRequestCapabilities(value: unknown): CustomJsxRequestCapability[] {
|
||||
if (!Array.isArray(value)) return [];
|
||||
return value.flatMap((candidate) => {
|
||||
@@ -34,10 +50,7 @@ export function parseRequestCapabilities(value: unknown): CustomJsxRequestCapabi
|
||||
method: record.method,
|
||||
trigger: record.trigger === "load" ? "load" : "manual",
|
||||
minimumBoardPermission: record.minimumBoardPermission,
|
||||
confirmation:
|
||||
record.confirmation && typeof record.confirmation === "object"
|
||||
? (record.confirmation as CustomJsxRequestCapability["confirmation"])
|
||||
: undefined,
|
||||
confirmation: parseConfirmation(record.confirmation),
|
||||
invalidates: Array.isArray(record.invalidates)
|
||||
? record.invalidates.filter((entry): entry is string => typeof entry === "string")
|
||||
: [],
|
||||
|
||||
@@ -75,6 +75,8 @@ export interface PreviewSessionStore {
|
||||
deleteSession(id: string): Promise<void>;
|
||||
appendJournal(id: string, value: unknown, maxEntries: number, ttlMs: number): Promise<void>;
|
||||
getJournal(id: string, maxEntries: number): Promise<unknown[]>;
|
||||
saveEvidence(id: string, key: string, value: unknown, ttlMs: number): Promise<void>;
|
||||
getEvidence(id: string): Promise<unknown[]>;
|
||||
}
|
||||
|
||||
export interface PreviewSessionServiceOptions {
|
||||
@@ -88,6 +90,7 @@ export interface PreviewSessionServiceOptions {
|
||||
export class CustomWidgetPreviewSessionService {
|
||||
private readonly sessions = new Map<string, CustomWidgetPreviewSession>();
|
||||
private readonly journals = new Map<string, CustomWidgetPreviewJournalEntry[]>();
|
||||
private readonly evidence = new Map<string, Map<string, CustomWidgetPreviewJournalEntry>>();
|
||||
private readonly now: () => number;
|
||||
|
||||
public constructor(private readonly options: PreviewSessionServiceOptions) {
|
||||
@@ -128,7 +131,11 @@ export class CustomWidgetPreviewSessionService {
|
||||
const parsed = sessionSchema.safeParse(parseStoredValue(candidate));
|
||||
if (!parsed.success || parsed.data.expiresAt <= this.now()) {
|
||||
if (this.options.store) await this.options.store.deleteSession(id);
|
||||
else this.sessions.delete(id);
|
||||
else {
|
||||
this.sessions.delete(id);
|
||||
this.journals.delete(id);
|
||||
this.evidence.delete(id);
|
||||
}
|
||||
throw new CustomWidgetDomainError({ code: "NOT_FOUND", message: "Preview session expired or was not found" });
|
||||
}
|
||||
if (parsed.data.userId !== userId) {
|
||||
@@ -164,8 +171,23 @@ export class CustomWidgetPreviewSessionService {
|
||||
): Promise<void> {
|
||||
const value = journalEntrySchema.parse({ ...input, id: this.options.createId(), timestamp: this.now() });
|
||||
const ttlMs = Math.max(1, session.expiresAt - this.now());
|
||||
if (this.options.store) await this.options.store.appendJournal(session.id, value, MAX_JOURNAL_ENTRIES, ttlMs);
|
||||
else this.journals.set(session.id, [value, ...(this.journals.get(session.id) ?? [])].slice(0, MAX_JOURNAL_ENTRIES));
|
||||
const isVerified =
|
||||
(value.kind === "query" && value.status !== null && value.status >= 200 && value.status < 300) ||
|
||||
(value.kind === "action" &&
|
||||
(value.simulated || (value.status !== null && value.status >= 200 && value.status < 300)));
|
||||
const evidenceKey = `${value.sessionRevision}:${value.kind}:${value.requestId}`;
|
||||
if (this.options.store) {
|
||||
const writes = [this.options.store.appendJournal(session.id, value, MAX_JOURNAL_ENTRIES, ttlMs)];
|
||||
if (isVerified) writes.push(this.options.store.saveEvidence(session.id, evidenceKey, value, ttlMs));
|
||||
await Promise.all(writes);
|
||||
return;
|
||||
}
|
||||
|
||||
this.journals.set(session.id, [value, ...(this.journals.get(session.id) ?? [])].slice(0, MAX_JOURNAL_ENTRIES));
|
||||
if (!isVerified) return;
|
||||
const evidence = this.evidence.get(session.id) ?? new Map<string, CustomWidgetPreviewJournalEntry>();
|
||||
evidence.set(evidenceKey, value);
|
||||
this.evidence.set(session.id, evidence);
|
||||
}
|
||||
|
||||
public async getJournal(id: string, userId: string): Promise<CustomWidgetPreviewJournalEntry[]> {
|
||||
@@ -179,6 +201,17 @@ export class CustomWidgetPreviewSessionService {
|
||||
});
|
||||
}
|
||||
|
||||
public async getEvidence(id: string, userId: string): Promise<CustomWidgetPreviewJournalEntry[]> {
|
||||
await this.get(id, userId);
|
||||
const entries = this.options.store
|
||||
? await this.options.store.getEvidence(id)
|
||||
: [...(this.evidence.get(id)?.values() ?? [])];
|
||||
return entries.flatMap((entry) => {
|
||||
const parsed = journalEntrySchema.safeParse(parseStoredValue(entry));
|
||||
return parsed.success ? [parsed.data] : [];
|
||||
});
|
||||
}
|
||||
|
||||
public getSecrets(session: CustomWidgetPreviewSession, sourceId: string) {
|
||||
return session.secrets
|
||||
.filter((secret) => secret.sourceId === sourceId)
|
||||
|
||||
@@ -134,6 +134,8 @@ describe("preview sessions", () => {
|
||||
},
|
||||
appendJournal: async () => undefined,
|
||||
getJournal: async () => [],
|
||||
saveEvidence: async () => undefined,
|
||||
getEvidence: async () => [],
|
||||
};
|
||||
const service = new CustomWidgetPreviewSessionService({
|
||||
createId: () => "session",
|
||||
|
||||
@@ -64,6 +64,13 @@ it("normalizes request capabilities and drops malformed entries", () => {
|
||||
confirmation: { title: "Approve", message: "Approve this request?" },
|
||||
invalidates: ["queue", 3],
|
||||
},
|
||||
{
|
||||
id: "malformed-confirmation",
|
||||
kind: "action",
|
||||
method: "POST",
|
||||
minimumBoardPermission: "modify",
|
||||
confirmation: { title: 3, message: "Approve this request?" },
|
||||
},
|
||||
{ id: "unsafe", kind: "action", method: "TRACE", minimumBoardPermission: "full" },
|
||||
]),
|
||||
).toEqual([
|
||||
@@ -76,6 +83,15 @@ it("normalizes request capabilities and drops malformed entries", () => {
|
||||
confirmation: { title: "Approve", message: "Approve this request?" },
|
||||
invalidates: ["queue"],
|
||||
},
|
||||
{
|
||||
id: "malformed-confirmation",
|
||||
kind: "action",
|
||||
method: "POST",
|
||||
trigger: "manual",
|
||||
minimumBoardPermission: "modify",
|
||||
confirmation: undefined,
|
||||
invalidates: [],
|
||||
},
|
||||
]);
|
||||
});
|
||||
|
||||
|
||||
Reference in new issue
Block a user