From 7bbf097fa8f05c0d8b18d839b06e93fd5dadadd6 Mon Sep 17 00:00:00 2001 From: Matt Rubens Date: Fri, 14 Feb 2025 08:40:35 -0500 Subject: [PATCH 1/2] Validate custom modes schema before creation from the UI --- src/core/config/CustomModesSchema.ts | 29 +++--- .../__tests__/CustomModesSchema.test.ts | 22 ----- .../__tests__/GroupConfigSchema.test.ts | 9 -- .../src/components/prompts/PromptsView.tsx | 99 +++++++++++++++---- webview-ui/src/index.css | 1 + 5 files changed, 93 insertions(+), 67 deletions(-) diff --git a/src/core/config/CustomModesSchema.ts b/src/core/config/CustomModesSchema.ts index 29f5f3eac90..4cf85cbe459 100644 --- a/src/core/config/CustomModesSchema.ts +++ b/src/core/config/CustomModesSchema.ts @@ -29,22 +29,19 @@ const GroupOptionsSchema = z.object({ const GroupEntrySchema = z.union([ToolGroupSchema, z.tuple([ToolGroupSchema, GroupOptionsSchema])]) // Schema for array of groups -const GroupsArraySchema = z - .array(GroupEntrySchema) - .min(1, "At least one tool group is required") - .refine( - (groups) => { - const seen = new Set() - return groups.every((group) => { - // For tuples, check the group name (first element) - const groupName = Array.isArray(group) ? group[0] : group - if (seen.has(groupName)) return false - seen.add(groupName) - return true - }) - }, - { message: "Duplicate groups are not allowed" }, - ) +const GroupsArraySchema = z.array(GroupEntrySchema).refine( + (groups) => { + const seen = new Set() + return groups.every((group) => { + // For tuples, check the group name (first element) + const groupName = Array.isArray(group) ? group[0] : group + if (seen.has(groupName)) return false + seen.add(groupName) + return true + }) + }, + { message: "Duplicate groups are not allowed" }, +) // Schema for mode configuration export const CustomModeSchema = z.object({ diff --git a/src/core/config/__tests__/CustomModesSchema.test.ts b/src/core/config/__tests__/CustomModesSchema.test.ts index 9af3ba71103..f46bac2c7ee 100644 --- a/src/core/config/__tests__/CustomModesSchema.test.ts +++ b/src/core/config/__tests__/CustomModesSchema.test.ts @@ -95,17 +95,6 @@ describe("CustomModeSchema", () => { expect(() => validateCustomMode(invalidGroupMode)).toThrow(ZodError) }) - test("rejects empty groups array", () => { - const invalidMode = { - slug: "123e4567-e89b-12d3-a456-426614174000", - name: "Test Mode", - roleDefinition: "Test role definition", - groups: [] as const, - } satisfies ModeConfig - - expect(() => validateCustomMode(invalidMode)).toThrow("At least one tool group is required") - }) - test("handles null and undefined gracefully", () => { expect(() => validateCustomMode(null)).toThrow(ZodError) expect(() => validateCustomMode(undefined)).toThrow(ZodError) @@ -179,16 +168,5 @@ describe("CustomModeSchema", () => { expect(() => CustomModeSchema.parse(modeWithDuplicates)).toThrow(/Duplicate groups/) }) - - it("requires at least one group", () => { - const modeWithNoGroups = { - slug: "test", - name: "Test", - roleDefinition: "Test", - groups: [], - } - - expect(() => CustomModeSchema.parse(modeWithNoGroups)).toThrow(/At least one tool group is required/) - }) }) }) diff --git a/src/core/config/__tests__/GroupConfigSchema.test.ts b/src/core/config/__tests__/GroupConfigSchema.test.ts index 3b1452b88d6..68d0f6b69a3 100644 --- a/src/core/config/__tests__/GroupConfigSchema.test.ts +++ b/src/core/config/__tests__/GroupConfigSchema.test.ts @@ -45,15 +45,6 @@ describe("GroupConfigSchema", () => { expect(() => CustomModeSchema.parse(mode)).toThrow() }) - test("rejects empty groups array", () => { - const mode = { - ...validBaseMode, - groups: [] as const, - } satisfies ModeConfig - - expect(() => CustomModeSchema.parse(mode)).toThrow("At least one tool group is required") - }) - test("rejects invalid group names", () => { const mode = { ...validBaseMode, diff --git a/webview-ui/src/components/prompts/PromptsView.tsx b/webview-ui/src/components/prompts/PromptsView.tsx index 5956ff6265b..51472d4cabb 100644 --- a/webview-ui/src/components/prompts/PromptsView.tsx +++ b/webview-ui/src/components/prompts/PromptsView.tsx @@ -19,6 +19,7 @@ import { ModeConfig, GroupEntry, } from "../../../../src/shared/modes" +import { CustomModeSchema } from "../../../../src/core/config/CustomModesSchema" import { supportPrompt, SupportPromptType, @@ -157,15 +158,34 @@ const PromptsView = ({ onDone }: PromptsViewProps) => { const [newModeGroups, setNewModeGroups] = useState(availableGroups) const [newModeSource, setNewModeSource] = useState("global") + // Field-specific error states + const [nameError, setNameError] = useState("") + const [slugError, setSlugError] = useState("") + const [roleDefinitionError, setRoleDefinitionError] = useState("") + const [groupsError, setGroupsError] = useState("") + + // Helper to reset form state + const resetFormState = useCallback(() => { + // Reset form fields + setNewModeName("") + setNewModeSlug("") + setNewModeGroups(availableGroups) + setNewModeRoleDefinition("") + setNewModeCustomInstructions("") + setNewModeSource("global") + // Reset error states + setNameError("") + setSlugError("") + setRoleDefinitionError("") + setGroupsError("") + }, []) + // Reset form fields when dialog opens useEffect(() => { if (isCreateModeDialogOpen) { - setNewModeGroups(availableGroups) - setNewModeRoleDefinition("") - setNewModeCustomInstructions("") - setNewModeSource("global") + resetFormState() } - }, [isCreateModeDialogOpen]) + }, [isCreateModeDialogOpen, resetFormState]) // Helper function to generate a unique slug from a name const generateSlug = useCallback((name: string, attempt = 0): string => { @@ -186,26 +206,52 @@ const PromptsView = ({ onDone }: PromptsViewProps) => { ) const handleCreateMode = useCallback(() => { - if (!newModeName.trim() || !newModeSlug.trim()) return + // Clear previous errors + setNameError("") + setSlugError("") + setRoleDefinitionError("") + setGroupsError("") const source = newModeSource const newMode: ModeConfig = { slug: newModeSlug, name: newModeName, - roleDefinition: newModeRoleDefinition.trim() || "", + roleDefinition: newModeRoleDefinition.trim(), customInstructions: newModeCustomInstructions.trim() || undefined, groups: newModeGroups, source, } + + // Validate the mode against the schema + const result = CustomModeSchema.safeParse(newMode) + if (!result.success) { + // Map Zod errors to specific fields + result.error.errors.forEach((error) => { + const field = error.path[0] as string + const message = error.message + + switch (field) { + case "name": + setNameError(message) + break + case "slug": + setSlugError(message) + break + case "roleDefinition": + setRoleDefinitionError(message) + break + case "groups": + setGroupsError(message) + break + } + }) + return + } + updateCustomMode(newModeSlug, newMode) switchMode(newModeSlug) setIsCreateModeDialogOpen(false) - setNewModeName("") - setNewModeSlug("") - setNewModeRoleDefinition("") - setNewModeCustomInstructions("") - setNewModeGroups(availableGroups) - setNewModeSource("global") + resetFormState() // eslint-disable-next-line react-hooks/exhaustive-deps }, [ newModeName, @@ -431,7 +477,7 @@ const PromptsView = ({ onDone }: PromptsViewProps) => {
e.stopPropagation()} className="flex justify-between items-center mb-3"> -

Mode-Specific Prompts

+

Modes

@@ -727,7 +773,7 @@ const PromptsView = ({ onDone }: PromptsViewProps) => { alignItems: "center", marginBottom: "4px", }}> -
Mode-specific Custom Instructions
+
Mode-specific Custom Instructions (optional)
{!findModeBySlug(mode, customModes) && ( { }} style={{ width: "100%" }} /> + {nameError && ( +
{nameError}
+ )}
Slug
@@ -1091,6 +1140,9 @@ const PromptsView = ({ onDone }: PromptsViewProps) => { The slug is used in URLs and file names. It should be lowercase and contain only letters, numbers, and hyphens.
+ {slugError && ( +
{slugError}
+ )}
Save Location
@@ -1147,6 +1199,11 @@ const PromptsView = ({ onDone }: PromptsViewProps) => { resize="vertical" style={{ width: "100%" }} /> + {roleDefinitionError && ( +
+ {roleDefinitionError} +
+ )}
Available Tools
@@ -1184,9 +1241,14 @@ const PromptsView = ({ onDone }: PromptsViewProps) => { ))}
+ {groupsError && ( +
{groupsError}
+ )}
-
Custom Instructions
+
+ Custom Instructions (optional) +
{ backgroundColor: "var(--vscode-editor-background)", }}> setIsCreateModeDialogOpen(false)}>Cancel - + Create Mode
diff --git a/webview-ui/src/index.css b/webview-ui/src/index.css index 7c83662c5f4..4e362460633 100644 --- a/webview-ui/src/index.css +++ b/webview-ui/src/index.css @@ -84,6 +84,7 @@ --color-vscode-notifications-background: var(--vscode-notifications-background); --color-vscode-notifications-border: var(--vscode-notifications-border); --color-vscode-descriptionForeground: var(--vscode-descriptionForeground); + --color-vscode-errorForeground: var(--vscode-errorForeground); } @layer base { From 5c9fea93f50c2d523a5a4f81feb43485cf74aa8f Mon Sep 17 00:00:00 2001 From: Matt Rubens Date: Fri, 14 Feb 2025 09:07:14 -0500 Subject: [PATCH 2/2] Fix some test warnings --- src/__mocks__/get-folder-size.js | 7 +++ src/core/config/CustomModesManager.ts | 2 - .../webview/__tests__/ClineProvider.test.ts | 46 ++++++++++--------- 3 files changed, 32 insertions(+), 23 deletions(-) diff --git a/src/__mocks__/get-folder-size.js b/src/__mocks__/get-folder-size.js index 9757854f581..082d5203deb 100644 --- a/src/__mocks__/get-folder-size.js +++ b/src/__mocks__/get-folder-size.js @@ -4,3 +4,10 @@ module.exports = async function getFolderSize() { errors: [], } } + +module.exports.loose = async function getFolderSizeLoose() { + return { + size: 1000, + errors: [], + } +} diff --git a/src/core/config/CustomModesManager.ts b/src/core/config/CustomModesManager.ts index 331e3fb9097..70107d9e692 100644 --- a/src/core/config/CustomModesManager.ts +++ b/src/core/config/CustomModesManager.ts @@ -62,8 +62,6 @@ export class CustomModesManager { const settings = JSON.parse(content) const result = CustomModesSettingsSchema.safeParse(settings) if (!result.success) { - const errorMsg = `Schema validation failed for ${filePath}` - console.error(`[CustomModesManager] ${errorMsg}:`, result.error) return [] } diff --git a/src/core/webview/__tests__/ClineProvider.test.ts b/src/core/webview/__tests__/ClineProvider.test.ts index 227c032a8fc..0f6612caa91 100644 --- a/src/core/webview/__tests__/ClineProvider.test.ts +++ b/src/core/webview/__tests__/ClineProvider.test.ts @@ -246,7 +246,7 @@ describe("ClineProvider", () => { // Mock CustomModesManager const mockCustomModesManager = { updateCustomMode: jest.fn().mockResolvedValue(undefined), - getCustomModes: jest.fn().mockResolvedValue({}), + getCustomModes: jest.fn().mockResolvedValue({ customModes: [] }), dispose: jest.fn(), } @@ -1049,7 +1049,7 @@ describe("ClineProvider", () => { "900x600", // browserViewportSize "code", // mode {}, // customModePrompts - {}, // customModes + { customModes: [] }, // customModes undefined, // effectiveInstructions undefined, // preferredLanguage true, // diffEnabled @@ -1102,7 +1102,7 @@ describe("ClineProvider", () => { "900x600", // browserViewportSize "code", // mode {}, // customModePrompts - {}, // customModes + { customModes: [] }, // customModes undefined, // effectiveInstructions undefined, // preferredLanguage false, // diffEnabled @@ -1220,12 +1220,14 @@ describe("ClineProvider", () => { provider.customModesManager = { updateCustomMode: jest.fn().mockResolvedValue(undefined), getCustomModes: jest.fn().mockResolvedValue({ - "test-mode": { - slug: "test-mode", - name: "Test Mode", - roleDefinition: "Updated role definition", - groups: ["read"] as const, - }, + customModes: [ + { + slug: "test-mode", + name: "Test Mode", + roleDefinition: "Updated role definition", + groups: ["read"] as const, + }, + ], }), dispose: jest.fn(), } as any @@ -1251,27 +1253,29 @@ describe("ClineProvider", () => { ) // Verify state was updated - expect(mockContext.globalState.update).toHaveBeenCalledWith( - "customModes", - expect.objectContaining({ - "test-mode": expect.objectContaining({ + expect(mockContext.globalState.update).toHaveBeenCalledWith("customModes", { + customModes: [ + expect.objectContaining({ slug: "test-mode", roleDefinition: "Updated role definition", }), - }), - ) + ], + }) // Verify state was posted to webview + // Verify state was posted to webview with correct format expect(mockPostMessage).toHaveBeenCalledWith( expect.objectContaining({ type: "state", state: expect.objectContaining({ - customModes: expect.objectContaining({ - "test-mode": expect.objectContaining({ - slug: "test-mode", - roleDefinition: "Updated role definition", - }), - }), + customModes: { + customModes: [ + expect.objectContaining({ + slug: "test-mode", + roleDefinition: "Updated role definition", + }), + ], + }, }), }), )