fix(custom-widgets): address final v2 review feedback (#6451)

apps/docs/docs/management/custom-widgets/troubleshooting.mdx

@@ -2,9 +2,11 @@
 
 ## Correct an unavailable widget
 
-The definition does not exist. Remove the unavailable tile from the board.
+The definition does not exist. If it was deleted, remove the unavailable tile from the board.
 
-Then, install or create a Custom JSX v2 definition. Homarr does not provide recovery or migration for old formats.
+If the Custom Widgets page shows the definition with a **Migration required** badge, open its action menu and select **Copy migration prompt**. Give that redacted prompt to an LLM, then import the resulting v2 JSON from the same menu. Homarr keeps the legacy definition until the replacement validates and you confirm the migration.
+
+Otherwise, install or create a Custom JSX v2 definition.
 
 ## Correct an invalid saved definition
 
@@ -12,9 +14,7 @@ The Custom Widgets page isolates a definition that does not conform to the curre
 
 An invalid definition shows its validation errors. You can delete it.
 
-You cannot edit, enable, export, duplicate, publish, or select an invalid definition. Recreate the definition or import a valid definition.
-
-Homarr does not migrate old Custom JSX formats.
+You cannot edit, enable, export, duplicate, publish, or select an invalid v2 definition. Recreate the definition or import a valid definition. Preserved v1 definitions use the separate **Migration required** workflow described above.
 
 ## Correct validation errors
 

apps/docs/src/components/workshop/SubmitForm.tsx

@@ -31,7 +31,6 @@ import { cn, errorMessage } from "@/lib/utils";
 
 import type { SubmitInput } from "./useWorkshop";
 import { ScreenshotEditor } from "./ScreenshotEditor";
-import { getJsonImportSubmissionType } from "./submission-import";
 
 interface Props {
   onClose: () => void;
@@ -105,7 +104,7 @@ export const SubmitForm = ({ onClose, onSubmit }: Props) => {
       const reader = new FileReader();
       reader.addEventListener("load", (e) => {
         const text = e.target?.result as string;
-        setType(getJsonImportSubmissionType(type));
+        setType("customWidget");
         setContentAndAutofill(text, title, description);
         if (step === 0) goTo(1);
       });
@@ -113,7 +112,7 @@ export const SubmitForm = ({ onClose, onSubmit }: Props) => {
       reader.readAsText(jsonFile);
       return true;
     },
-    [type, step, title, description, setContentAndAutofill, goTo],
+    [step, title, description, setContentAndAutofill, goTo],
   );
 
   const addImageFiles = useCallback(

apps/docs/src/components/workshop/submission-import.spec.ts

@@ -1,13 +0,0 @@
-import { describe, expect, it } from "vitest";
-
-import { getJsonImportSubmissionType } from "./submission-import";
-
-describe("Workshop JSON import classification", () => {
-  it("switches an existing Custom CSS draft to Custom Widget", () => {
-    expect(getJsonImportSubmissionType("customCss")).toBe("customWidget");
-  });
-
-  it("classifies JSON before a submission type is selected", () => {
-    expect(getJsonImportSubmissionType(null)).toBe("customWidget");
-  });
-});

apps/docs/src/components/workshop/submission-import.ts

@@ -1,3 +0,0 @@
-import type { SubmissionType } from "@site/src/lib/workshop-schema";
-
-export const getJsonImportSubmissionType = (_currentType: SubmissionType | null): SubmissionType => "customWidget";

apps/nextjs/src/app/[locale]/manage/custom-widgets/_custom-widget-actions.tsx

@@ -67,16 +67,23 @@ export const CustomWidgetRowActions = ({ widget }: { widget: WidgetRef }) => {
     if (!file) return;
     const reader = new FileReader();
     reader.addEventListener("load", (loadEvent) => {
-      const result = parseCustomWidgetClipboardDetailed(loadEvent.target?.result as string);
-      if (!result.success) {
+      try {
+        const result = parseCustomWidgetClipboardDetailed(loadEvent.target?.result as string);
+        if (!result.success) {
+          showErrorNotification({
+            title: t("action.migrate"),
+            message: formatCustomWidgetImportIssues(result.issues),
+          });
+          return;
+        }
+        setMigratedWidget(result.widget);
+        migrationControls.open();
+      } catch {
         showErrorNotification({
           title: t("action.migrate"),
-          message: formatCustomWidgetImportIssues(result.issues),
+          message: t("notification.migrationError"),
         });
-        return;
       }
-      setMigratedWidget(result.widget);
-      migrationControls.open();
     });
     reader.addEventListener("error", () => {
       showErrorNotification({ title: t("action.migrate"), message: t("notification.migrationError") });

apps/nextjs/src/components/workshop/workshop-publish-definition.spec.ts

@@ -1,8 +1,8 @@
-import { readFileSync } from "node:fs";
-import { describe, expect, it } from "vitest";
+import { describe, expect, it, vi } from "vitest";
 
 import {
   getPrivateWorkshopSourceNames,
+  publishWorkshopDefinition,
   serializeWorkshopDefinition,
   workshopDefinitionChanged,
 } from "./workshop-publish-definition";
@@ -26,16 +26,27 @@ describe("Workshop publish definition inspection", () => {
     expect(workshopDefinitionChanged(definition, { ...definition, name: "Updated" })).toBe(true);
   });
 
-  it("submits the same refreshed export that passed the change check", () => {
-    const source = readFileSync(
-      `${process.cwd()}/apps/nextjs/src/components/workshop/workshop-publish-modal.tsx`,
-      "utf8",
-    );
-    expect(source).toContain("customWidget.export.useQuery");
-    expect(source).toContain("await definition.refetch()");
-    expect(source).toContain("workshopDefinitionChanged(inspectedDefinition, refreshed.data)");
-    expect(source).toContain("content: serializeWorkshopDefinition(refreshed.data)");
-    expect(source).toMatch(/disabled=\{[\s\S]*!definition\.data[\s\S]*definition\.isError/u);
-    expect(serializeWorkshopDefinition(definition)).toBe(JSON.stringify(definition));
+  it("blocks publishing when the definition changes after inspection", async () => {
+    const publish = vi.fn(async () => undefined);
+    await expect(
+      publishWorkshopDefinition({
+        inspectedDefinition: definition,
+        refetchDefinition: async () => ({ ...definition, name: "Updated" }),
+        publish,
+      }),
+    ).resolves.toBe("changed");
+    expect(publish).not.toHaveBeenCalled();
+  });
+
+  it("publishes the same refreshed export that passed the change check", async () => {
+    const publish = vi.fn(async () => undefined);
+    await expect(
+      publishWorkshopDefinition({
+        inspectedDefinition: definition,
+        refetchDefinition: async () => definition,
+        publish,
+      }),
+    ).resolves.toBe("published");
+    expect(publish).toHaveBeenCalledWith(serializeWorkshopDefinition(definition));
   });
 });

apps/nextjs/src/components/workshop/workshop-publish-definition.ts

@@ -11,6 +11,22 @@ export const serializeWorkshopDefinition = (definition: unknown): string => {
 export const workshopDefinitionChanged = (previous: unknown, current: unknown) =>
   serializeWorkshopDefinition(previous) !== serializeWorkshopDefinition(current);
 
+export async function publishWorkshopDefinition({
+  inspectedDefinition,
+  refetchDefinition,
+  publish,
+}: {
+  inspectedDefinition: unknown;
+  refetchDefinition: () => Promise<unknown | undefined>;
+  publish: (content: string) => Promise<unknown>;
+}) {
+  const refreshedDefinition = await refetchDefinition();
+  if (refreshedDefinition === undefined) return "unavailable" as const;
+  if (workshopDefinitionChanged(inspectedDefinition, refreshedDefinition)) return "changed" as const;
+  await publish(serializeWorkshopDefinition(refreshedDefinition));
+  return "published" as const;
+}
+
 export const getPrivateWorkshopSourceNames = (definition: PublishableWorkshopDefinition | undefined) =>
   Object.entries(definition?.sources ?? {}).flatMap(([sourceId, source]) =>
     source.networkScope === "public" ? [] : [source.name ?? sourceId],

apps/nextjs/src/components/workshop/workshop-publish-modal.tsx

@@ -12,8 +12,8 @@ import { useScopedI18n } from "@homarr/translation/client";
 import { createWorkshopClient } from "./workshop-client";
 import {
   getPrivateWorkshopSourceNames,
+  publishWorkshopDefinition,
   serializeWorkshopDefinition,
-  workshopDefinitionChanged,
 } from "./workshop-publish-definition";
 
 export function WorkshopPublishModal({
@@ -60,22 +60,26 @@ export function WorkshopPublishModal({
     try {
       if (!definition.data) throw new Error(t("publish.error"));
       const inspectedDefinition = definition.data;
-      const refreshed = await definition.refetch();
-      if (!refreshed.data) throw new Error(t("publish.error"));
-      if (workshopDefinitionChanged(inspectedDefinition, refreshed.data)) {
+      const result = await publishWorkshopDefinition({
+        inspectedDefinition,
+        refetchDefinition: async () => (await definition.refetch()).data,
+        publish: (content) =>
+          createSubmission.mutateAsync({
+            input: {
+              type: "customWidget",
+              title,
+              description,
+              content,
+            },
+            screenshots,
+          }),
+      });
+      if (result === "unavailable") throw new Error(t("publish.error"));
+      if (result === "changed") {
         setSourceUrlsReviewed(false);
-        setError(t("publish.error"));
+        setError(t("publish.definitionChanged"));
         return;
       }
-      await createSubmission.mutateAsync({
-        input: {
-          type: "customWidget",
-          title,
-          description,
-          content: serializeWorkshopDefinition(refreshed.data),
-        },
-        screenshots,
-      });
       setPublished(true);
     } catch (cause) {
       setError(cause instanceof Error ? cause.message : t("publish.error"));

apps/workshop/pb_hooks/workshop-utils.js

@@ -7,11 +7,6 @@ const rejectRequest = (message) => {
   throw new BadRequestError(message);
 };
 
-const requestBodyValue = (event, name) => {
-  const body = event.requestInfo().body;
-  return body && typeof body.get === "function" ? body.get(name) : body ? body[name] : undefined;
-};
-
 const validateWidgetManifest = (content) => {
   let widget;
   try {
@@ -87,7 +82,7 @@ const sendEmail = (app, recipientEmail, subject, text, html) => {
 module.exports = {
   emailTemplate,
   escapeHtml,
-  requestBodyValue,
+  rejectRequest,
   sendEmail,
   validateAndNormalizeSubmission,
 };

apps/workshop/pb_hooks/workshop.pb.js

@@ -15,20 +15,22 @@ onRecordCreateRequest((event) => {
   const { validateAndNormalizeSubmission } = require(`${__hooks}/workshop-utils.js`);
   validateAndNormalizeSubmission(event.record);
   event.record.set("revision", 1);
+  event.record.set("expectedRevision", 0);
   event.record.set("changelog", "");
   event.record.set("outdated", false);
   event.next();
 }, "submissions");
 
 onRecordUpdateRequest((event) => {
-  const { requestBodyValue, validateAndNormalizeSubmission } = require(`${__hooks}/workshop-utils.js`);
+  const { rejectRequest, validateAndNormalizeSubmission } = require(`${__hooks}/workshop-utils.js`);
   const original = event.record.original();
   const currentRevision = original.getInt("revision");
-  const expectedRevision = Number(requestBodyValue(event, "expectedRevision"));
-  if (!Number.isSafeInteger(expectedRevision) || expectedRevision !== currentRevision) {
+  const expectedRevision = event.record.getInt("expectedRevision");
+  if (!Number.isSafeInteger(expectedRevision) || expectedRevision < 1) {
     rejectRequest("Submission changed since it was read");
   }
   validateAndNormalizeSubmission(event.record);
+  event.record.set("expectedRevision", expectedRevision);
   event.record.set("revision", currentRevision + 1);
   event.next();
 }, "submissions");

apps/workshop/pb_migrations/1784240000_workshop_widgets.js

@@ -88,6 +88,7 @@ migrate(
         { type: "text", name: "widgetSchema", required: true, min: 1, max: 100 },
         { type: "text", name: "content", required: true, max: 1000000 },
         { type: "number", name: "revision", required: true, onlyInt: true, min: 1 },
+        { type: "number", name: "expectedRevision", onlyInt: true, min: 0 },
         { type: "text", name: "changelog", max: 2000 },
         { type: "bool", name: "outdated" },
         {
@@ -112,9 +113,9 @@ migrate(
         `CREATE TRIGGER submissions_revision_cas
          BEFORE UPDATE ON submissions
          FOR EACH ROW
-         WHEN NEW.revision != OLD.revision + 1
+         WHEN NEW.expectedRevision != OLD.revision OR NEW.revision != OLD.revision + 1
          BEGIN
-           SELECT RAISE(ABORT, 'submission revision must increment exactly once');
+           SELECT RAISE(ABORT, 'submission revision compare-and-swap failed');
          END`,
       )
       .execute();
@@ -285,8 +286,6 @@ migrate(
         if (field) users.fields.removeById(field.id);
       }
       app.save(users);
-    } else {
-      app.delete(users);
     }
     app.delete(stateCollection);
   },

apps/workshop/tests/workshop-contracts.mjs

@@ -9,7 +9,19 @@ if (typeof clientExport !== "string") throw new Error("Workshop client export is
 await access(resolve("packages/workshop", clientExport));
 
 const hook = await read("apps/workshop/pb_hooks/workshop.pb.js");
-await access(resolve("apps/workshop/pb_hooks/workshop-utils.js"));
+const hookUtils = await read("apps/workshop/pb_hooks/workshop-utils.js");
+const schema = await read("packages/workshop/src/schema.ts");
+const constantValue = (source, name) => {
+  const match = source.match(new RegExp(`(?:export )?const ${name} = ([\\d_]+)`));
+  if (!match?.[1]) throw new Error(`Workshop limit ${name} is missing or is not a numeric literal`);
+  return Number(match[1].replaceAll("_", ""));
+};
+if (constantValue(hookUtils, "MAX_CSS_LENGTH") !== constantValue(schema, "MAX_WORKSHOP_CSS_LENGTH")) {
+  throw new Error("PocketBase and shared Workshop CSS limits must match");
+}
+if (constantValue(hookUtils, "MAX_CONTENT_LENGTH") !== constantValue(schema, "MAX_WORKSHOP_CONTENT_LENGTH")) {
+  throw new Error("PocketBase and shared Workshop content limits must match");
+}
 if (!hook.includes("require(`${__hooks}/workshop-utils.js`)")) {
   throw new Error("Workshop handlers must load shared helpers inside their isolated PocketBase contexts");
 }
@@ -25,6 +37,7 @@ for (const required of [
   "workshop_migration_state",
   "addedUserFields",
   "CREATE TRIGGER submissions_revision_cas",
+  "NEW.expectedRevision != OLD.revision",
   "state.rateLimits.enabled",
   "users.passwordAuth = state.users.passwordAuth",
   "users.oauth2 = state.users.oauth2",

apps/workshop/tests/workshop-migration.mjs

@@ -125,7 +125,13 @@ migration.up(app);
 if (!collections.has("workshop_migration_state") || settings.rateLimits.enabled !== true) {
   throw new Error("Workshop migration did not capture state before applying changes");
 }
-if (!rawQueries.some((query) => query.includes("CREATE TRIGGER submissions_revision_cas"))) {
+if (
+  !rawQueries.some(
+    (query) =>
+      query.includes("CREATE TRIGGER submissions_revision_cas") &&
+      query.includes("NEW.expectedRevision != OLD.revision"),
+  )
+) {
   throw new Error("Workshop migration did not install the atomic submission revision guard");
 }
 migration.down(app);
@@ -145,4 +151,23 @@ for (const name of ["workshop_migration_state", "workshop_listings", "reports",
   if (collections.has(name)) throw new Error(`Rollback left collection ${name}`);
 }
 
+collections.clear();
+records.clear();
+rawQueries.length = 0;
+settings.rateLimits = structuredClone(original.rateLimits);
+
+migration.up(app);
+const bootstrappedUsers = collections.get("users");
+if (!bootstrappedUsers) throw new Error("Fresh migration did not create the users collection");
+migration.down(app);
+if (collections.get("users") !== bootstrappedUsers) {
+  throw new Error("Fresh rollback removed the users collection required by the bootstrap hook");
+}
+for (const name of ["workshop_migration_state", "workshop_listings", "reports", "comments", "votes", "submissions"]) {
+  if (collections.has(name)) throw new Error(`Fresh rollback left collection ${name}`);
+}
+if (JSON.stringify(settings.rateLimits) !== JSON.stringify(original.rateLimits)) {
+  throw new Error("Fresh rollback did not restore rate limits");
+}
+
 console.log("Workshop migration up/down restoration passed");

packages/api/src/router/custom-widget/management-queries.ts

@@ -47,21 +47,30 @@ export const managementQueryProcedures = {
       return [...current, ...legacy].toSorted((left, right) => left.name.localeCompare(right.name));
     }),
 
-  legacyMigrationPrompt: manageProcedure.input(z.object({ id: z.string() })).query(async ({ ctx, input }) => {
-    const definition = await ctx.db.query.legacyCustomWidgetDefinitions.findFirst({
-      where: eq(legacyCustomWidgetDefinitions.id, input.id),
-      with: { secrets: true },
-    });
-    if (!definition) throw new TRPCError({ code: "NOT_FOUND", message: "Legacy custom widget not found" });
-    return {
-      id: definition.id,
-      version: 1 as const,
-      prompt: buildLegacyCustomWidgetMigrationPrompt(
-        definition,
-        definition.secrets.map(({ kind }) => kind),
-      ),
-    };
-  }),
+  legacyMigrationPrompt: manageProcedure
+    .meta({
+      mcp: {
+        enabled: true,
+        description:
+          "Get a redacted AI migration prompt for one preserved legacy Custom Widget. Credentials must be configured separately.",
+      },
+    })
+    .input(z.object({ id: z.string() }))
+    .query(async ({ ctx, input }) => {
+      const definition = await ctx.db.query.legacyCustomWidgetDefinitions.findFirst({
+        where: eq(legacyCustomWidgetDefinitions.id, input.id),
+        with: { secrets: true },
+      });
+      if (!definition) throw new TRPCError({ code: "NOT_FOUND", message: "Legacy custom widget not found" });
+      return {
+        id: definition.id,
+        version: 1 as const,
+        prompt: buildLegacyCustomWidgetMigrationPrompt(
+          definition,
+          definition.secrets.map(({ kind }) => kind),
+        ),
+      };
+    }),
 
   get: manageProcedure
     .meta({ mcp: { enabled: true, description: "Get one Custom JSX widget without secret values." } })

packages/api/src/router/custom-widget/transfer-procedures.ts

@@ -40,6 +40,13 @@ export const transferProcedures = {
     }),
 
   migrateLegacy: manageProcedure
+    .meta({
+      mcp: {
+        enabled: true,
+        description:
+          "Replace one preserved legacy Custom Widget with a validated v2 definition while retaining compatible encrypted credentials.",
+      },
+    })
     .input(
       z.object({
         id: z.string(),
@@ -124,10 +131,18 @@ export const transferProcedures = {
       return { id: legacy.id, preservedSecretCount: preservedSecrets.length };
     }),
 
-  deleteLegacy: manageProcedure.input(z.object({ id: z.string() })).mutation(async ({ ctx, input }) => {
-    await ctx.db.delete(legacyCustomWidgetDefinitions).where(eq(legacyCustomWidgetDefinitions.id, input.id));
-    logger.info("Deleted legacy custom widget definition", { id: input.id });
-  }),
+  deleteLegacy: manageProcedure
+    .meta({
+      mcp: {
+        enabled: true,
+        description: "Permanently delete one preserved legacy Custom Widget that will not be migrated.",
+      },
+    })
+    .input(z.object({ id: z.string() }))
+    .mutation(async ({ ctx, input }) => {
+      await ctx.db.delete(legacyCustomWidgetDefinitions).where(eq(legacyCustomWidgetDefinitions.id, input.id));
+      logger.info("Deleted legacy custom widget definition", { id: input.id });
+    }),
 };
 
 function canPreserveLegacySecrets(

packages/api/src/test/mcp.spec.ts

@@ -22,6 +22,7 @@ const MCP_TOOL_ALLOWLIST = {
     "customWidget_get",
     "customWidget_getAuthoringPrompt",
     "customWidget_getSkill",
+    "customWidget_legacyMigrationPrompt",
     "customWidget_list",
     "customWidget_previewJournal",
     "customWidget_previewQuery",
@@ -69,6 +70,7 @@ const MCP_TOOL_ALLOWLIST = {
     "board_setHomeBoard",
     "board_setMobileHomeBoard",
     "customWidget_delete",
+    "customWidget_deleteLegacy",
     "customWidget_previewAction",
     "customWidget_templatePatch",
     "dnsHole_disable",
@@ -94,6 +96,7 @@ const MCP_TOOL_ALLOWLIST = {
     "apiKeys_delete",
     "customWidget_configurationRequestUser",
     "customWidget_create",
+    "customWidget_migrateLegacy",
     "customWidget_previewCreate",
     "customWidget_secretSet",
     "customWidget_sourceConfigure",

packages/custom-widgets/src/server/request-executor.ts

@@ -70,7 +70,6 @@ async function performRequest(input: CustomWidgetHttpRequest): Promise<CustomWid
   try {
     return await performRequestWithinDeadline(input, controller.signal);
   } catch (error) {
-    if (error instanceof CustomWidgetDomainError) throw error;
     if (controller.signal.aborted) {
       throw new CustomWidgetDomainError({
         code: "BAD_GATEWAY",
@@ -78,6 +77,7 @@ async function performRequest(input: CustomWidgetHttpRequest): Promise<CustomWid
         cause: error,
       });
     }
+    if (error instanceof CustomWidgetDomainError) throw error;
     throw error;
   } finally {
     clearTimeout(timeout);

packages/translation/src/lang/en.json

@@ -1181,6 +1181,7 @@
       "privateSourceWarning": "This widget contains private source URLs for: {sources}. Replace personal hosts or IP addresses with safe service placeholders before publishing.",
       "privateSourceConfirmation": "I reviewed these source URLs and they do not expose private infrastructure details",
       "action": "Publish now",
+      "definitionChanged": "The widget changed while publication was being prepared. Review its private source URLs again, then retry.",
       "error": "Publication failed"
     },
     "installDialog": "Install from Workshop",

review.md

@@ -6,6 +6,17 @@ The range contains 630 changed files, 54,851 insertions, and 20,056 deletions. T
 
 Follow-up status: the accompanying `feat/fix-v2-review` branch addresses every verified finding below. It preserves v1 Custom Widgets as migration-required records, provides a secret-redacted LLM migration workflow with atomic replacement, and adds regression coverage across the affected runtime, Workshop, documentation, MCP, database, and CI surfaces.
 
+## Post-PR reviewer follow-up
+
+After the initial fixes were published, CodeRabbit produced eight inline comments and five lower-priority test/maintainability suggestions before its `v2` base-branch policy skipped later review runs. Each was rechecked against the updated branch:
+
+- Legacy troubleshooting now directs users to the preserved-v1, LLM-assisted migration flow, and migration-file parsing reports unexpected failures.
+- Workshop publication distinguishes a changed definition from a load failure; behavioral tests prove a changed refetch cannot publish and an unchanged refetch publishes the exact reviewed JSON.
+- Workshop optimistic concurrency is enforced by a SQLite compare-and-swap trigger using the request's expected revision, with repeated real PocketBase concurrent-update coverage.
+- Workshop rollback preserves a bootstrapped `users` collection on fresh installs, tests both pre-existing and fresh-user paths, and statically enforces parity between shared and PocketBase content limits.
+- The legacy migration prompt, migration mutation, and legacy deletion are included in the reviewed MCP allowlist with permission-aware classifications.
+- Total request deadlines take precedence over wrapped domain errors, the obsolete OpenAPI import is already replaced by the passing local-source parity check, the unsaved-guard spec matches its module name, and the one-line Workshop JSON classification wrapper was removed.
+
 ## Original merge recommendation
 
 Do not merge the reviewed `origin/v2` range without the accompanying fixes. The database reset is release-blocking because it irreversibly deletes every existing custom widget and credential. Legacy widgets must remain visible as migration-required records, with a safe LLM-assisted conversion path to v2. The credential-retargeting paths, Custom JSX denial-of-service paths, Workshop server-validation bypass, and stale preview publication race should also be fixed before merge.