From a6fc7f82d3c23b68c3e3bdaf57b16e08062b1290 Mon Sep 17 00:00:00 2001 From: iliya Date: Fri, 20 Mar 2026 18:41:01 +0200 Subject: [PATCH] =?UTF-8?q?feat:=20tool=20approval=20improvements=20?= =?UTF-8?q?=E2=80=94=20diff=20preview,=20error=20feedback,=20provisioning?= =?UTF-8?q?=20fix?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add diff preview for Write/Edit tools: reads current file via new IPC (TEAM_TOOL_APPROVAL_READ_FILE), shows unified diff using existing DiffViewer - Fix Allow button doing nothing: re-throw errors from store instead of silently swallowing, show error message in UI, add 10s safety timeout - Fix "No active process" during provisioning: use getTrackedRunId() to find process in both provisioning and alive maps - Add 5s stdin.write timeout to prevent hanging when process dies - Add syntax highlighting for tool input preview (JSON, bash, etc.) - Add team color/name badge from ToolApprovalRequest (works during provisioning) - Conditionally show team badge only when user is on a different team page - Format elapsed time as Xm Ys when over 60 seconds - Replace native - void updateSettings({ timeoutAction: e.target.value as ToolApprovalTimeoutAction }) + onValueChange={(value) => + void updateSettings({ timeoutAction: value as ToolApprovalTimeoutAction }) } - className="rounded border px-1.5 py-0.5 text-xs" - style={{ - backgroundColor: 'var(--color-surface-raised)', - borderColor: 'var(--color-border)', - color: 'var(--color-text)', - }} > - - - - + + + + + Wait forever + Allow + Deny + + {settings.timeoutAction !== 'wait' && ( <> diff --git a/src/renderer/hooks/useToolApprovalDiff.ts b/src/renderer/hooks/useToolApprovalDiff.ts new file mode 100644 index 00000000..787e7aab --- /dev/null +++ b/src/renderer/hooks/useToolApprovalDiff.ts @@ -0,0 +1,212 @@ +import { useEffect, useRef, useState } from 'react'; + +import { api } from '@renderer/api'; + +import type { ToolApprovalFileContent } from '@shared/types'; + +// ============================================================================= +// Types +// ============================================================================= + +export interface ToolApprovalDiffData { + /** Whether this tool type supports diff preview */ + hasDiff: boolean; + /** Loading state (file read in progress) */ + loading: boolean; + /** Error message */ + error: string | null; + /** File name for syntax highlighting */ + fileName: string; + /** Original file content */ + oldString: string; + /** New file content after tool execution */ + newString: string; + /** File doesn't exist yet */ + isNewFile: boolean; + /** File content was truncated at size limit */ + truncated: boolean; + /** File is binary */ + isBinary: boolean; +} + +const DIFF_TOOLS = new Set(['Write', 'Edit', 'NotebookEdit']); + +const INITIAL_STATE: ToolApprovalDiffData = { + hasDiff: false, + loading: false, + error: null, + fileName: '', + oldString: '', + newString: '', + isNewFile: false, + truncated: false, + isBinary: false, +}; + +// ============================================================================= +// Helpers +// ============================================================================= + +function getFilePath(toolInput: Record): string { + const fp = toolInput.file_path ?? toolInput.notebook_path; + return typeof fp === 'string' ? fp : ''; +} + +function computeEditResult( + currentContent: string, + toolInput: Record +): { newString: string; error: string | null } { + const oldStr = typeof toolInput.old_string === 'string' ? toolInput.old_string : ''; + const newStr = typeof toolInput.new_string === 'string' ? toolInput.new_string : ''; + const replaceAll = toolInput.replace_all === true; + + if (!oldStr) { + return { newString: currentContent, error: 'Edit: old_string is empty' }; + } + + if (!currentContent.includes(oldStr)) { + return { newString: currentContent, error: 'Fragment not found in current file' }; + } + + const result = replaceAll + ? currentContent.replaceAll(oldStr, newStr) + : currentContent.replace(oldStr, newStr); + + return { newString: result, error: null }; +} + +// ============================================================================= +// Hook +// ============================================================================= + +/** + * Lazy-loading hook that reads file content from disk and computes diff data + * for Write/Edit/NotebookEdit tool approvals. + * + * @param toolName - The tool requesting approval + * @param toolInput - The tool's input parameters + * @param requestId - Unique approval request ID (used for cancellation) + * @param enabled - Only fetch when true (lazy — user expanded the diff section) + */ +export function useToolApprovalDiff( + toolName: string, + toolInput: Record, + requestId: string, + enabled: boolean +): ToolApprovalDiffData { + const [state, setState] = useState(INITIAL_STATE); + const activeRef = useRef(null); + + const hasDiff = DIFF_TOOLS.has(toolName); + const filePath = getFilePath(toolInput); + + useEffect(() => { + // Reset when approval changes + setState(INITIAL_STATE); + activeRef.current = null; + }, [requestId]); + + useEffect(() => { + if (!hasDiff || !enabled || !filePath) return; + + // NotebookEdit: no file read needed, just show new_source + if (toolName === 'NotebookEdit') { + const newSource = typeof toolInput.new_source === 'string' ? toolInput.new_source : ''; + setState({ + hasDiff: true, + loading: false, + error: null, + fileName: filePath, + oldString: '', + newString: newSource, + isNewFile: false, + truncated: false, + isBinary: false, + }); + return; + } + + // Write / Edit: need to read current file from disk + const currentRequestId = requestId; + activeRef.current = currentRequestId; + + setState((prev) => ({ ...prev, hasDiff: true, loading: true, error: null })); + + void (async () => { + let result: ToolApprovalFileContent; + try { + result = await api.teams.readFileForToolApproval(filePath); + } catch (err) { + if (activeRef.current !== currentRequestId) return; + setState((prev) => ({ + ...prev, + loading: false, + error: err instanceof Error ? err.message : String(err), + })); + return; + } + + if (activeRef.current !== currentRequestId) return; + + if (result.error) { + setState((prev) => ({ + ...prev, + loading: false, + error: result.error ?? 'Unknown read error', + fileName: filePath, + })); + return; + } + + if (result.isBinary) { + setState((prev) => ({ + ...prev, + loading: false, + isBinary: true, + fileName: filePath, + })); + return; + } + + const isNewFile = !result.exists; + const currentContent = result.content; + + if (toolName === 'Write') { + const newContent = typeof toolInput.content === 'string' ? toolInput.content : ''; + setState({ + hasDiff: true, + loading: false, + error: null, + fileName: filePath, + oldString: currentContent, + newString: newContent, + isNewFile, + truncated: result.truncated, + isBinary: false, + }); + } else if (toolName === 'Edit') { + const { newString, error } = computeEditResult(currentContent, toolInput); + setState({ + hasDiff: true, + loading: false, + error, + fileName: filePath, + oldString: currentContent, + newString, + isNewFile: false, + truncated: result.truncated, + isBinary: false, + }); + } + })(); + + return () => { + activeRef.current = null; + }; + // eslint-disable-next-line react-hooks/exhaustive-deps -- toolInput is a fresh object each render, use requestId for identity + }, [hasDiff, enabled, filePath, requestId, toolName]); + + if (!hasDiff) return INITIAL_STATE; + + return { ...state, hasDiff: true }; +} diff --git a/src/renderer/store/slices/teamSlice.ts b/src/renderer/store/slices/teamSlice.ts index af1995ea..c7c8bf6c 100644 --- a/src/renderer/store/slices/teamSlice.ts +++ b/src/renderer/store/slices/teamSlice.ts @@ -1231,6 +1231,16 @@ export const createTeamSlice: StateCreator = (set, return; } + // Draft team: team.meta.json exists but config.json doesn't (provisioning failed) + if (msg === 'TEAM_DRAFT' || msg.includes('TEAM_DRAFT')) { + set({ + selectedTeamLoading: false, + selectedTeamData: null, + selectedTeamError: 'TEAM_DRAFT', + }); + return; + } + const message = error instanceof IpcError ? error.message @@ -1299,6 +1309,15 @@ export const createTeamSlice: StateCreator = (set, return; } + if (msg === 'TEAM_DRAFT' || msg.includes('TEAM_DRAFT')) { + set({ + selectedTeamLoading: false, + selectedTeamData: null, + selectedTeamError: 'TEAM_DRAFT', + }); + return; + } + logger.warn(`refreshTeamData(${teamName}) failed: ${msg}`); // Non-destructive: if we already have data, keep it visible. @@ -2061,9 +2080,11 @@ export const createTeamSlice: StateCreator = (set, (a) => !(a.runId === runId && a.requestId === requestId) ), })); - } catch { - // IPC failed — approval stays in UI, user can retry - // Do NOT modify pendingApprovals — nothing was removed, nothing to rollback + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + logger.error(`respondToToolApproval failed for ${teamName}/${requestId}: ${msg}`); + // Surface the error so ToolApprovalSheet can show feedback + throw err; } }, diff --git a/src/shared/types/api.ts b/src/shared/types/api.ts index f19ba5f7..14674d56 100644 --- a/src/shared/types/api.ts +++ b/src/shared/types/api.ts @@ -74,6 +74,7 @@ import type { TeamTaskStatus, TeamUpdateConfigRequest, ToolApprovalEvent, + ToolApprovalFileContent, ToolApprovalSettings, UpdateKanbanPatch, } from './team'; @@ -419,6 +420,8 @@ export interface TeamsAPI { deleteTeam: (teamName: string) => Promise; restoreTeam: (teamName: string) => Promise; permanentlyDeleteTeam: (teamName: string) => Promise; + getSavedRequest: (teamName: string) => Promise; + deleteDraft: (teamName: string) => Promise; prepareProvisioning: (cwd?: string) => Promise; createTeam: (request: TeamCreateRequest) => Promise; getProvisioningStatus: (runId: string) => Promise; @@ -536,6 +539,7 @@ export interface TeamsAPI { validateCliArgs: (rawArgs: string) => Promise; onToolApprovalEvent: (callback: (event: unknown, data: ToolApprovalEvent) => void) => () => void; updateToolApprovalSettings: (settings: ToolApprovalSettings) => Promise; + readFileForToolApproval: (filePath: string) => Promise; } // ============================================================================= diff --git a/src/shared/types/team.ts b/src/shared/types/team.ts index 1d016592..82a525d0 100644 --- a/src/shared/types/team.ts +++ b/src/shared/types/team.ts @@ -53,6 +53,8 @@ export interface TeamSummary { sessionHistory?: string[]; /** Propagated from config.deletedAt — set when the team has been soft-deleted. */ deletedAt?: string; + /** True when team.meta.json exists but config.json doesn't — provisioning failed before TeamCreate. */ + pendingCreate?: boolean; } export type TeamTaskStatus = 'pending' | 'in_progress' | 'completed' | 'deleted'; @@ -803,6 +805,10 @@ export interface ToolApprovalRequest { toolInput: Record; /** ISO timestamp when the request was received. */ receivedAt: string; + /** Team color name (from config or create request) for badge rendering. */ + teamColor?: string; + /** Team display name (from config or create request). */ + teamDisplayName?: string; } /** Dismissal event — process died, all pending approvals for this team+run should be removed. */ @@ -853,3 +859,12 @@ export type ToolApprovalEvent = | ToolApprovalRequest | ToolApprovalDismiss | ToolApprovalAutoResolved; + +/** Result of reading a file for tool approval diff preview. */ +export interface ToolApprovalFileContent { + content: string; + exists: boolean; + truncated: boolean; + isBinary: boolean; + error?: string; +}