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
This commit is contained in:
Toray Altas 2026-01-16 19:46:42 -05:00
parent e079e6c7aa
commit ff4d057f1e
8 changed files with 465 additions and 33 deletions

View file

@ -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

View file

@ -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,

View file

@ -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<string, ResolvedHook>()
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<string, ResolvedHook>()
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", () => {

View file

@ -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<boolean> => {
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}`)
//

View file

@ -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<void> {
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(() => {})
}
}

View file

@ -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 = () => {
<SectionHeader>{t("settings:sections.hooks")}</SectionHeader>
<Section>
{/* Enable all hooks */}
{/* Description paragraph */}
<div
style={{
color: "var(--vscode-foreground)",
fontSize: "13px",
marginBottom: "10px",
marginTop: "5px",
}}>
{t("settings:hooks.description")}
</div>
{/* Enable all hooks - matching MCP checkbox styling */}
{enabledHooks.length > 0 && (
<div className="flex items-center justify-between gap-3 mb-4 p-3 rounded border border-vscode-input-border bg-vscode-input-background">
<div className="flex flex-col">
<span className="text-sm font-medium">{t("settings:hooks.enableHooks")}</span>
<span className="text-xs text-vscode-descriptionForeground">
{t("settings:hooks.enableHooksDescription")}
</span>
</div>
<label className="flex items-center gap-2 cursor-pointer flex-shrink-0">
<div style={{ marginBottom: "20px" }}>
<label className="flex items-center gap-2 cursor-pointer">
<input
type="checkbox"
checked={allHooksEnabled}
@ -99,8 +104,16 @@ export const HooksSettings: React.FC = () => {
onChange={(e) => handleToggleAllHooks(e.target.checked)}
className="w-4 h-4 cursor-pointer"
/>
<span className="text-sm">{t("settings:hooks.enabled")}</span>
<span style={{ fontWeight: "500" }}>{t("settings:hooks.enableHooks")}</span>
</label>
<p
style={{
fontSize: "12px",
marginTop: "5px",
color: "var(--vscode-descriptionForeground)",
}}>
{t("settings:hooks.enableHooksDescription")}
</p>
</div>
)}
@ -251,9 +264,20 @@ const HookItem: React.FC<HookItemProps> = ({ hook, onToggle }) => {
return () => window.removeEventListener("message", handleMessage)
}, [hook.id])
const handleToggle = (e: React.ChangeEvent<HTMLInputElement>) => {
e.stopPropagation()
onToggle(hook.id, e.target.checked)
const handleToggleEnabled = () => {
onToggle(hook.id, !hook.enabled)
}
const handleDeleteHook = () => {
vscode.postMessage({
type: "hooksDeleteHook",
hookId: hook.id,
hooksSource: hook.source,
})
}
const getEnabledDotColor = () => {
return hook.enabled ? "var(--vscode-testing-iconPassed)" : "var(--vscode-descriptionForeground)"
}
return (
@ -281,17 +305,34 @@ const HookItem: React.FC<HookItemProps> = ({ hook, onToggle }) => {
{hook.source}
</span>
</div>
<label
className="flex items-center gap-2 cursor-pointer flex-shrink-0"
onClick={(e) => e.stopPropagation()}>
<input
type="checkbox"
checked={hook.enabled}
onChange={handleToggle}
className="w-4 h-4 cursor-pointer"
<div className="flex items-center gap-2 flex-shrink-0" onClick={(e) => e.stopPropagation()}>
<Button
variant="ghost"
size="icon"
onClick={handleDeleteHook}
data-testid={`hook-delete-${hook.id}`}
aria-label={t("settings:hooks.deleteHook")}
style={{ marginRight: "6px" }}>
<span className="codicon codicon-trash" style={{ fontSize: "14px" }}></span>
</Button>
<div
data-testid={`hook-status-dot-${hook.id}`}
style={{
width: "8px",
height: "8px",
borderRadius: "50%",
background: getEnabledDotColor(),
marginLeft: "2px",
}}
/>
<span className="text-sm">{t("settings:hooks.enabled")}</span>
</label>
<ToggleSwitch
checked={hook.enabled}
onChange={handleToggleEnabled}
size="medium"
aria-label={t("settings:hooks.enabled")}
data-testid={`hook-enabled-toggle-${hook.id}`}
/>
</div>
</div>
{/* Expanded Content */}

View file

@ -48,6 +48,9 @@ vi.mock("@src/components/ui", () => ({
{children}
</button>
),
ToggleSwitch: ({ checked, onChange, ...props }: any) => (
<div role="switch" aria-checked={checked} onClick={onChange} data-testid={props["data-testid"]} />
),
StandardTooltip: ({ children, content }: any) => <div title={content}>{children}</div>,
}))
@ -79,10 +82,17 @@ describe("HooksSettings", () => {
render(<HooksSettings />)
expect(screen.getByText("settings:sections.hooks")).toBeInTheDocument()
expect(screen.getByText("settings:hooks.description")).toBeInTheDocument()
expect(screen.getByText("settings:hooks.noHooksConfigured")).toBeInTheDocument()
expect(screen.getByText("settings:hooks.noHooksHint")).toBeInTheDocument()
})
it("renders description paragraph at the top", () => {
render(<HooksSettings />)
expect(screen.getByText("settings:hooks.description")).toBeInTheDocument()
})
it("renders hooks list when hooks are configured", () => {
const mockHook: HookInfo = {
id: "hook-1",
@ -358,9 +368,7 @@ describe("HooksSettings", () => {
render(<HooksSettings />)
// First checkbox is the top-level "Enable Hooks" toggle; the second is the per-hook toggle in collapsed header
const checkboxes = screen.getAllByRole("checkbox")
fireEvent.click(checkboxes[1])
fireEvent.click(screen.getByTestId("hook-enabled-toggle-hook-1"))
expect(vscode.postMessage).toHaveBeenCalledWith({
type: "hooksSetEnabled",
@ -391,9 +399,7 @@ describe("HooksSettings", () => {
// Hook should be collapsed initially
expect(screen.queryByText(mockHook.event)).not.toBeInTheDocument()
// Click the checkbox (second checkbox, first is "Enable Hooks")
const checkboxes = screen.getAllByRole("checkbox")
fireEvent.click(checkboxes[1])
fireEvent.click(screen.getByTestId("hook-enabled-toggle-hook-1"))
// Hook should still be collapsed after toggling
expect(screen.queryByText(mockHook.event)).not.toBeInTheDocument()
@ -406,6 +412,57 @@ describe("HooksSettings", () => {
})
})
it("renders green status dot in collapsed row", () => {
const mockHook: HookInfo = {
id: "hook-1",
event: "before_execute_command",
commandPreview: "echo test",
enabled: true,
source: "global",
timeout: 30,
}
currentHooksState = {
enabledHooks: [mockHook],
executionHistory: [],
hasProjectHooks: false,
}
render(<HooksSettings />)
const dot = screen.getByTestId("hook-status-dot-hook-1")
expect(dot).toBeInTheDocument()
expect(dot).toHaveStyle({ background: "var(--vscode-testing-iconPassed)" })
})
it("sends hooksDeleteHook message when trash button is clicked", async () => {
const { vscode } = await import("@src/utils/vscode")
const mockHook: HookInfo = {
id: "hook-1",
event: "before_execute_command",
commandPreview: "echo test",
enabled: true,
source: "project",
timeout: 30,
}
currentHooksState = {
enabledHooks: [mockHook],
executionHistory: [],
hasProjectHooks: false,
}
render(<HooksSettings />)
fireEvent.click(screen.getByTestId("hook-delete-hook-1"))
expect(vscode.postMessage).toHaveBeenCalledWith({
type: "hooksDeleteHook",
hookId: "hook-1",
hooksSource: "project",
})
})
it("sends hooksSetAllEnabled message when top-level Enable Hooks toggle is changed", async () => {
const { vscode } = await import("@src/utils/vscode")

View file

@ -44,6 +44,7 @@
"about": "About Roo Code"
},
"hooks": {
"description": "Hooks execute custom shell commands automatically when Roo uses specific tools. Use them to integrate with external systems, enforce workflows, or automate repetitive tasks.",
"configuredHooks": "Configured Hooks",
"lastLoadedTooltip": "Last loaded at {{time}}",
"reloadTooltip": "Reload hooks configuration from disk",
@ -68,7 +69,6 @@
"enabled": "Enabled",
"event": "Event",
"matcher": "Matcher",
"description": "Description",
"command": "Command",
"shell": "Shell",
"timeout": "Timeout",