From ff4d057f1e054f1a45d422f89163e4d28bb95524 Mon Sep 17 00:00:00 2001 From: Toray Altas Date: Fri, 16 Jan 2026 19:46:42 -0500 Subject: [PATCH] feat: enhance hooks UI with delete support and status indicators - Add switch, status dot, and delete button to HooksSettings UI - Implement backend support for hook deletion - Add safeWriteText utility - Update translation strings - Add UI and backend tests for new functionality --- packages/types/src/vscode-extension-host.ts | 7 +- src/core/webview/ClineProvider.ts | 1 + .../webviewMessageHandler.hooks.spec.ts | 129 ++++++++++++++++++ src/core/webview/webviewMessageHandler.ts | 107 +++++++++++++++ src/utils/safeWriteText.ts | 94 +++++++++++++ .../src/components/settings/HooksSettings.tsx | 89 ++++++++---- .../settings/__tests__/HooksSettings.spec.tsx | 69 +++++++++- webview-ui/src/i18n/locales/en/settings.json | 2 +- 8 files changed, 465 insertions(+), 33 deletions(-) create mode 100644 src/utils/safeWriteText.ts diff --git a/packages/types/src/vscode-extension-host.ts b/packages/types/src/vscode-extension-host.ts index d69f5c7141..29c772833e 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -224,6 +224,8 @@ export interface HookExecutionStatusPayload { export interface HookInfo { /** Unique identifier for this hook */ id: string + /** File path where this hook was defined (if known) */ + filePath?: string /** The event type this hook is registered for */ event: string /** Tool name filter (regex/glob pattern) */ @@ -619,6 +621,7 @@ export interface WebviewMessage { | "hooksSetEnabled" | "hooksSetAllEnabled" | "hooksOpenConfigFolder" + | "hooksDeleteHook" text?: string editedMessageContent?: string tab?: "settings" | "history" | "mcp" | "modes" | "chat" | "marketplace" | "cloud" @@ -674,10 +677,10 @@ export interface WebviewMessage { list?: string[] // For dismissedUpsells response organizationId?: string | null // For organization switching useProviderSignup?: boolean // For rooCloudSignIn to use provider signup flow - hookId?: string // For hooksSetEnabled + hookId?: string // For hooksSetEnabled, hooksDeleteHook hookEnabled?: boolean // For hooksSetEnabled hooksEnabled?: boolean // For hooksSetAllEnabled - hooksSource?: "global" | "project" // For hooksOpenConfigFolder + hooksSource?: "global" | "project" | "mode" // For hooksOpenConfigFolder, hooksDeleteHook codeIndexSettings?: { // Global state settings codebaseIndexEnabled: boolean diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 55aaa313b2..047bebbc15 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -2251,6 +2251,7 @@ export class ClineProvider // Convert ResolvedHook[] to HookInfo[] const hookInfos = allHooks.map((hook) => ({ id: hook.id, + filePath: hook.filePath, event: hook.event, matcher: hook.matcher, commandPreview: hook.command.length > 100 ? hook.command.substring(0, 97) + "..." : hook.command, diff --git a/src/core/webview/__tests__/webviewMessageHandler.hooks.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.hooks.spec.ts index b6d6f693c4..804fb2a927 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.hooks.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.hooks.spec.ts @@ -27,12 +27,30 @@ vi.mock("vscode", () => { vi.mock("fs/promises", () => { const mockMkdir = vi.fn().mockResolvedValue(undefined) + const mockReadFile = vi.fn().mockResolvedValue("") + const mockWriteFile = vi.fn().mockResolvedValue(undefined) + const mockAccess = vi.fn().mockResolvedValue(undefined) + const mockRename = vi.fn().mockResolvedValue(undefined) + const mockUnlink = vi.fn().mockResolvedValue(undefined) + const mockReaddir = vi.fn().mockResolvedValue([]) return { default: { mkdir: mockMkdir, + readFile: mockReadFile, + writeFile: mockWriteFile, + access: mockAccess, + rename: mockRename, + unlink: mockUnlink, + readdir: mockReaddir, }, mkdir: mockMkdir, + readFile: mockReadFile, + writeFile: mockWriteFile, + access: mockAccess, + rename: mockRename, + unlink: mockUnlink, + readdir: mockReaddir, } }) @@ -40,11 +58,20 @@ vi.mock("../../../utils/fs", () => ({ fileExistsAtPath: vi.fn().mockResolvedValue(true), })) +vi.mock("../../../utils/safeWriteJson", () => ({ + safeWriteJson: vi.fn().mockResolvedValue(undefined), +})) + +vi.mock("../../../utils/safeWriteText", () => ({ + safeWriteText: vi.fn().mockResolvedValue(undefined), +})) + vi.mock("../../../api/providers/fetchers/modelCache") import * as vscode from "vscode" import * as fs from "fs/promises" import * as fsUtils from "../../../utils/fs" +import { safeWriteJson } from "../../../utils/safeWriteJson" import { webviewMessageHandler } from "../webviewMessageHandler" import type { ClineProvider } from "../ClineProvider" @@ -376,6 +403,108 @@ describe("webviewMessageHandler - hooks commands", () => { expect(vscode.window.showErrorMessage).toHaveBeenCalledWith("Failed to open hooks configuration folder") }) }) + + describe("hooksDeleteHook", () => { + it("should delete hook from JSON config file and then reload + post state", async () => { + const hookId = "hook-to-delete" + const hookFilePath = "/mock/workspace/.roo/hooks/hooks.json" + + const hooksById = new Map() + hooksById.set(hookId, { + id: hookId, + event: "PreToolUse" as any, + matcher: ".*", + command: "echo hi", + enabled: true, + source: "project" as any, + timeout: 30, + filePath: hookFilePath, + includeConversationHistory: false, + } as any) + + vi.mocked(mockHookManager.getConfigSnapshot).mockReturnValue({ + hooksByEvent: new Map(), + hooksById, + loadedAt: new Date(), + disabledHookIds: new Set(), + hasProjectHooks: true, + } as HooksConfigSnapshot) + + vi.mocked(fs.readFile).mockResolvedValueOnce( + JSON.stringify({ + version: "1", + hooks: { + PreToolUse: [ + { id: hookId, command: "echo hi" }, + { id: "keep", command: "echo keep" }, + ], + }, + }), + ) + + await webviewMessageHandler(mockClineProvider, { + type: "hooksDeleteHook", + hookId, + } as any) + + expect(safeWriteJson).toHaveBeenCalledWith( + hookFilePath, + expect.objectContaining({ + version: "1", + hooks: { + PreToolUse: [{ id: "keep", command: "echo keep" }], + }, + }), + ) + expect(mockHookManager.reloadHooksConfig).toHaveBeenCalledTimes(1) + expect(mockClineProvider.postStateToWebview).toHaveBeenCalledTimes(1) + }) + + it("should show error and not reload when hook is not found in config file", async () => { + const hookId = "missing-hook" + const hookFilePath = "/mock/workspace/.roo/hooks/hooks.json" + + const hooksById = new Map() + hooksById.set(hookId, { + id: hookId, + event: "PreToolUse" as any, + matcher: ".*", + command: "echo hi", + enabled: true, + source: "project" as any, + timeout: 30, + filePath: hookFilePath, + includeConversationHistory: false, + } as any) + + vi.mocked(mockHookManager.getConfigSnapshot).mockReturnValue({ + hooksByEvent: new Map(), + hooksById, + loadedAt: new Date(), + disabledHookIds: new Set(), + hasProjectHooks: true, + } as HooksConfigSnapshot) + + vi.mocked(fs.readFile).mockResolvedValueOnce( + JSON.stringify({ + version: "1", + hooks: { + PreToolUse: [{ id: "keep", command: "echo keep" }], + }, + }), + ) + + await webviewMessageHandler(mockClineProvider, { + type: "hooksDeleteHook", + hookId, + } as any) + + expect(safeWriteJson).not.toHaveBeenCalled() + expect(mockHookManager.reloadHooksConfig).not.toHaveBeenCalled() + expect(mockClineProvider.postStateToWebview).not.toHaveBeenCalled() + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith("Failed to delete hook") + }) + }) }) describe("webviewMessageHandler - hooks state integration", () => { diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index e39d0e3245..b5867f0449 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -1,4 +1,5 @@ import { safeWriteJson } from "../../utils/safeWriteJson" +import { safeWriteText } from "../../utils/safeWriteText" import * as path from "path" import * as os from "os" import * as fs from "fs/promises" @@ -3428,6 +3429,112 @@ export const webviewMessageHandler = async ( break } + case "hooksDeleteHook": { + const hookManager = provider.getHookManager() + if (!hookManager || !message.hookId) { + break + } + + try { + const hookId = message.hookId + const snapshot = hookManager.getConfigSnapshot() + const targetHook = snapshot?.hooksById.get(hookId) + // Prefer resolved snapshot filePath (ResolvedHook.filePath) + const targetFilePath = targetHook?.filePath + + const removeHookFromFile = async (filePath: string): Promise => { + const lower = filePath.toLowerCase() + const content = await fs.readFile(filePath, "utf-8") + const parsed = lower.endsWith(".json") + ? JSON.parse(content) + : (await import("yaml")).default.parse(content) + + if (!parsed || typeof parsed !== "object") { + throw new Error(`Invalid hooks config format in ${filePath}`) + } + + const hooks = (parsed as any).hooks + if (!hooks || typeof hooks !== "object") { + return false + } + + let removed = false + for (const [event, defs] of Object.entries(hooks)) { + if (!Array.isArray(defs)) continue + const before = defs.length + const after = defs.filter((d: any) => d?.id !== hookId) + if (after.length !== before) { + removed = true + ;(hooks as any)[event] = after + } + } + + if (!removed) return false + + if (lower.endsWith(".json")) { + await safeWriteJson(filePath, parsed) + } else { + const YAML = (await import("yaml")).default + const newYaml = YAML.stringify(parsed, { lineWidth: 0 }) + await safeWriteText(filePath, newYaml) + } + + return true + } + + let deleted = false + if (typeof targetFilePath === "string" && targetFilePath.length > 0) { + deleted = await removeHookFromFile(targetFilePath) + } else { + // Fallback: scan all loaded roo directories for hook configs and remove matching id. + const cwd = provider.cwd + const rooDirs = getRooDirectoriesForCwd(cwd) + const candidateDirs: string[] = [] + candidateDirs.push(path.join(rooDirs[0], "hooks")) + if (message.hooksSource === "mode") { + const mode = (await provider.getState()).mode + candidateDirs.push(path.join(rooDirs[1], `hooks-${mode}`)) + } + candidateDirs.push(path.join(rooDirs[1], "hooks")) + + for (const dir of candidateDirs) { + let entries: any[] = [] + try { + entries = await fs.readdir(dir, { withFileTypes: true }) + } catch { + continue + } + const files = entries + .filter((e) => e.isFile()) + .map((e) => path.join(dir, e.name)) + .filter((p) => { + const l = p.toLowerCase() + return l.endsWith(".json") || l.endsWith(".yaml") || l.endsWith(".yml") + }) + .sort() + for (const filePath of files) { + if (await removeHookFromFile(filePath)) { + deleted = true + break + } + } + if (deleted) break + } + } + + if (!deleted) { + throw new Error("Hook not found in any loaded config file") + } + + await hookManager.reloadHooksConfig() + await provider.postStateToWebview() + } catch (error) { + provider.log(`Failed to delete hook: ${error instanceof Error ? error.message : String(error)}`) + vscode.window.showErrorMessage("Failed to delete hook") + } + break + } + default: { // console.log(`Unhandled message type: ${message.type}`) // diff --git a/src/utils/safeWriteText.ts b/src/utils/safeWriteText.ts new file mode 100644 index 0000000000..7f16fde7c9 --- /dev/null +++ b/src/utils/safeWriteText.ts @@ -0,0 +1,94 @@ +import * as fs from "fs/promises" +import * as path from "path" +import * as lockfile from "proper-lockfile" + +/** + * Safely writes text data to a file. + * - Creates parent directories if they don't exist + * - Uses 'proper-lockfile' for inter-process advisory locking to prevent concurrent writes + * - Writes to a temporary file in the same directory first + * - If the target file exists, it's backed up before being replaced + * - Attempts to roll back and clean up in case of errors + */ +export async function safeWriteText(filePath: string, content: string): Promise { + const absoluteFilePath = path.resolve(filePath) + let releaseLock = async () => {} + + const dirPath = path.dirname(absoluteFilePath) + await fs.mkdir(dirPath, { recursive: true }) + await fs.access(dirPath) + + releaseLock = await lockfile.lock(absoluteFilePath, { + stale: 31000, + update: 10000, + realpath: false, + retries: { + retries: 5, + factor: 2, + minTimeout: 100, + maxTimeout: 1000, + }, + onCompromised: (err) => { + throw err + }, + }) + + let tempNewPath: string | null = null + let tempBackupPath: string | null = null + + try { + tempNewPath = path.join( + path.dirname(absoluteFilePath), + `.${path.basename(absoluteFilePath)}.new_${Date.now()}_${Math.random().toString(36).substring(2)}.tmp`, + ) + + await fs.writeFile(tempNewPath, content, "utf8") + + try { + await fs.access(absoluteFilePath) + tempBackupPath = path.join( + path.dirname(absoluteFilePath), + `.${path.basename(absoluteFilePath)}.bak_${Date.now()}_${Math.random().toString(36).substring(2)}.tmp`, + ) + await fs.rename(absoluteFilePath, tempBackupPath) + } catch (accessError: any) { + if (accessError.code !== "ENOENT") { + throw accessError + } + } + + await fs.rename(tempNewPath, absoluteFilePath) + tempNewPath = null + + if (tempBackupPath) { + try { + await fs.unlink(tempBackupPath) + tempBackupPath = null + } catch { + // non-fatal + } + } + } catch (originalError) { + // Attempt rollback if backup was made + if (tempBackupPath) { + try { + await fs.rename(tempBackupPath, absoluteFilePath) + tempBackupPath = null + } catch { + // If rollback fails, preserve original error. + } + } + + if (tempNewPath) { + await fs.unlink(tempNewPath).catch(() => {}) + } + + if (tempBackupPath) { + await fs.unlink(tempBackupPath).catch(() => {}) + } + + throw originalError + } finally { + await releaseLock().catch(() => {}) + } +} diff --git a/webview-ui/src/components/settings/HooksSettings.tsx b/webview-ui/src/components/settings/HooksSettings.tsx index 28c55a6fa2..febae3228a 100644 --- a/webview-ui/src/components/settings/HooksSettings.tsx +++ b/webview-ui/src/components/settings/HooksSettings.tsx @@ -3,7 +3,7 @@ import { RefreshCw, FolderOpen, AlertTriangle, Clock, Zap, X } from "lucide-reac import { useAppTranslation } from "@src/i18n/TranslationContext" import { useExtensionState } from "@src/context/ExtensionStateContext" import { vscode } from "@src/utils/vscode" -import { Button, StandardTooltip } from "@src/components/ui" +import { Button, StandardTooltip, ToggleSwitch } from "@src/components/ui" import type { HookInfo, HookExecutionRecord, HookExecutionStatusPayload } from "@roo-code/types" import { SectionHeader } from "./SectionHeader" import { Section } from "./Section" @@ -82,16 +82,21 @@ export const HooksSettings: React.FC = () => { {t("settings:sections.hooks")}
- {/* Enable all hooks */} + {/* Description paragraph */} +
+ {t("settings:hooks.description")} +
+ + {/* Enable all hooks - matching MCP checkbox styling */} {enabledHooks.length > 0 && ( -
-
- {t("settings:hooks.enableHooks")} - - {t("settings:hooks.enableHooksDescription")} - -
-
-