diff --git a/Auto Run Docs/Refactor-20251219.md b/Auto Run Docs/Refactor-20251219.md index 006265ff..75465527 100644 --- a/Auto Run Docs/Refactor-20251219.md +++ b/Auto Run Docs/Refactor-20251219.md @@ -271,7 +271,7 @@ - Excellent type safety - no `any` types - Recommended 7 refactoring actions prioritized by impact -- [ ] **Task 14:** `src/main/ipc/handlers/agents.ts` +- [x] **Task 14:** `src/main/ipc/handlers/agents.ts` Agent management IPC handlers. Examine for: - Dead: Unused agent management handlers - Deprecated: Legacy agent API patterns @@ -279,7 +279,16 @@ - Smells: Agent state management issues - Types: Agent type definitions -- [ ] **Task 15:** `src/main/ipc/handlers/autorun.ts` + **Completed:** See `Auto Run Docs/Refactor/14.md` for full audit. Key findings: + - File is well-structured (382 lines) with all 16 handlers actively used + - `AgentConfigsData` interface duplicated in 6 locations across codebase + - Config access pattern `agentConfigsStore.get('configs', {})` repeated 13 times + - 3 `any` types in `stripAgentFunctions` that need proper typing + - `AgentConfig` interface duplicated in 8+ locations (see also Task 4) + - No dead code or deprecated patterns found + - Recommended 7 refactoring actions prioritized by impact + +- [x] **Task 15:** `src/main/ipc/handlers/autorun.ts` Auto Run IPC handlers. Examine for: - Dead: Unused Auto Run handlers - Deprecated: Old file handling patterns @@ -287,7 +296,18 @@ - Smells: Missing error handling, path validation - Types: File/document type safety -- [ ] **Task 16:** `src/main/ipc/handlers/claude.ts` + **Completed:** See `Auto Run Docs/Refactor/15.md` for full audit. Key findings: + - File is well-structured (499 lines) with good test coverage and proper path validation security + - Debug `console.log` and `[DEBUG]` logging left in production code (lines 171-175) + - `TreeNode` interface duplicates `src/shared/treeUtils.ts`, should import from shared + - Image extension arrays duplicated twice within file and 2+ other locations across codebase + - Path validation logic duplicated inline in `saveImage`/`deleteImage` despite helper existing + - Unused `mainWindow` parameter in function signature (only `getMainWindow` used) + - Single global debounce timer could cause interference between watched folders + - Excellent type safety - no `any` types found + - Recommended 7 refactoring actions prioritized by impact + +- [x] **Task 16:** `src/main/ipc/handlers/claude.ts` Claude-specific handlers. Examine for: - Dead: Claude handlers that should use generic agent API - Deprecated: Claude-specific patterns that should be generic @@ -295,7 +315,18 @@ - Smells: Tight coupling to Claude implementation - Types: Claude-specific type leakage -- [ ] **Task 17:** `src/main/ipc/handlers/git.ts` + **Completed:** See `Auto Run Docs/Refactor/16.md` for full audit. Key findings: + - Entire file (1,566 lines) is deprecated legacy code - should migrate to `agentSessions:*` API + - Cost calculation formula duplicated 4 times within file (16 occurrences of `1_000_000` magic number) + - Session content parsing logic duplicated 4+ times across codebase + - `ClaudeSessionOrigin` types duplicated in 4 separate files + - 30 empty/silent catch blocks that mask errors + - `claude:getGlobalStats` handler is ~200 lines God method + - 6 uses of `unknown` type that should have proper typing + - Still actively used by 10+ renderer call sites (migration required) + - Recommended 7 refactoring actions prioritized by impact + +- [x] **Task 17:** `src/main/ipc/handlers/git.ts` Git IPC handlers. Examine for: - Dead: Unused git operations - Deprecated: Old git command patterns @@ -303,6 +334,16 @@ - Smells: Shell injection risks, error handling - Types: Git result typing + **Completed:** See `Auto Run Docs/Refactor/17.md` for full audit. Key findings: + - File is well-structured (579 lines) with good delegation to shared/gitUtils.ts + - No dead code - all 17 handlers are exposed via preload.ts and actively used + - Runtime `require('child_process')` should be ES import at file top + - `WorktreeConfig` interface duplicated in shared/types.ts and renderer/types/index.ts + - **Bug: preload.ts type mismatches** - `git:status` and `git:diff` declare `Promise` but handlers return `{ stdout, stderr }` + - Log entry result missing `additions`/`deletions` fields in preload.ts type + - `git:worktreeSetup` handler is 107 lines - should be extracted to service class + - Recommended 7 refactoring actions prioritized by impact + - [ ] **Task 18:** `src/main/ipc/handlers/groupChat.ts` Group chat handlers. Examine for: - Dead: Unused group chat operations diff --git a/Auto Run Docs/Refactor/17.md b/Auto Run Docs/Refactor/17.md new file mode 100644 index 00000000..02880bfa --- /dev/null +++ b/Auto Run Docs/Refactor/17.md @@ -0,0 +1,181 @@ +# Refactor Task 17: `src/main/ipc/handlers/git.ts` + +## Summary + +The Git IPC handlers file is well-structured (579 lines) with proper use of shared utilities and helper abstractions. The file demonstrates good separation of concerns by delegating parsing logic to `src/shared/gitUtils.ts`. However, there are some notable issues: a runtime `require()` for `child_process`, return type inconsistencies in the preload API, worktree result type duplication, and complex handler logic in worktree operations. No dead code was found - all handlers are exposed and actively used. + +## Dead Code + +None found. All 17 git IPC handlers are: +1. Registered in `registerGitHandlers()` +2. Exposed via `preload.ts` (lines 284-345) +3. Called from renderer via `window.maestro.git.*` + +All handlers: +- `git:status`, `git:diff`, `git:isRepo`, `git:numstat` - Basic operations, used by `gitService.ts` +- `git:branch`, `git:branches`, `git:tags`, `git:remote` - Branch/tag operations, used for autocompletion +- `git:info`, `git:log`, `git:commitCount`, `git:show`, `git:showFile` - Advanced queries, used by GitLogViewer/MainPanel +- `git:worktreeInfo`, `git:getRepoRoot`, `git:worktreeSetup`, `git:worktreeCheckout` - Worktree operations, used by useBatchProcessor +- `git:createPR`, `git:checkGhCli`, `git:getDefaultBranch` - GitHub CLI integration + +## Deprecated Patterns + +1. **Runtime `require()` for child_process** (line 236-237) + ```typescript + const { spawnSync } = require('child_process'); + ``` + - Should use ES module import at file top + - The reason for runtime require is unclear (possibly circular dependency or lazy loading) + - Creates TypeScript type inference issues + +2. **Inconsistent handler wrapper usage** + - Some handlers use `withIpcErrorLogging()` while others use `createIpcHandler()` + - Both do similar things but `createIpcHandler` adds more structure + - Handlers: `worktreeInfo`, `getRepoRoot`, `getDefaultBranch` use `createIpcHandler` + - All others use `withIpcErrorLogging` + +## Duplication + +1. **`WorktreeConfig` interface duplicated** (2 locations) + - `src/shared/types.ts:94` - Primary definition + - `src/renderer/types/index.ts:114` - Duplicate definition + +2. **Worktree result types defined inline in preload.ts** + - `worktreeInfo`, `worktreeSetup`, `worktreeCheckout`, `getRepoRoot`, `getDefaultBranch` each have inline return types in preload.ts (lines 1138-1177) + - These should be extracted to shared types and reused + +3. **Git existence check pattern repeated** (3 times in git.ts) + ```typescript + const result = await execFileNoThrow('git', [...], cwd); + if (result.exitCode !== 0) { ... } + ``` + - Lines 93-95 (branches), 107-109 (tags), 163-167 (log) + - Could be abstracted to a helper + +4. **Path existence check pattern** in worktree handlers + ```typescript + try { await fs.access(path); } catch { return { exists: false, ... }; } + ``` + - Lines 267-272 (worktreeInfo), 359-365 (worktreeSetup) + +## Code Smells + +1. **`git:worktreeSetup` handler is 107 lines** (lines 337-443) + - Complex logic for: + - Path resolution and validation + - Nested worktree detection + - Existing path handling + - Branch existence checking + - Worktree creation with or without new branch + - Should be extracted to a dedicated worktree service class + +2. **`git:log` handler has complex inline parsing** (lines 143-201) + - 58 lines with inline parsing logic + - Parsing logic could be moved to `src/shared/gitUtils.ts` alongside other parsers + +3. **Magic strings for git commands scattered throughout** + - `--porcelain`, `--abbrev-ref`, `rev-parse`, etc. + - While standard git commands, key patterns like commit format strings should be constants: + ```typescript + '--pretty=format:COMMIT_START%H|%an|%ad|%D|%s' // line 153 + ``` + +4. **Missing proper error handling in git:showFile** (lines 226-259) + - Binary file handling uses `spawnSync` which can throw synchronously + - No try/catch around the spawnSync call (line 237) + +5. **Hard-coded buffer size** (line 240) + ```typescript + maxBuffer: 50 * 1024 * 1024 // 50MB max + ``` + - Should be a named constant + +## Type Safety Issues + +1. **Implicit `any` from require()** (line 236) + ```typescript + const { spawnSync } = require('child_process'); + ``` + - TypeScript cannot properly type this at runtime + - Should use: `import { spawnSync } from 'child_process';` + +2. **Return type mismatch between handler and preload.ts** + - `git:status` handler returns `{ stdout, stderr }` (line 41) + - But `preload.ts:1111` declares: `status: (cwd: string) => Promise` + - This is a bug - the type should be `Promise<{ stdout: string; stderr: string }>` + +3. **Similar mismatch for git:diff** + - Handler returns `{ stdout, stderr }` (line 51) + - Preload declares `Promise` (line 1112) + +4. **Complex inline types for worktree results** + - `git:worktreeInfo` returns 5 optional properties (lines 315-320) + - `git:worktreeSetup` returns 6 optional properties (lines 436-442) + - These should be named interfaces in shared types + +5. **Missing type for log entry options** (line 145) + ```typescript + options?: { limit?: number; search?: string } + ``` + - Should be a named interface `GitLogOptions` + +6. **Log entry result missing `additions` and `deletions`** in preload.ts + - Handler returns `additions` and `deletions` (lines 194-195) + - Preload type (lines 1125-1132) does not include these fields + +## Recommended Refactoring + +1. **High Priority - Fix preload.ts type mismatches** + - Update `git:status` return type from `Promise` to `Promise<{ stdout: string; stderr: string }>` + - Update `git:diff` return type from `Promise` to `Promise<{ stdout: string; stderr: string }>` + - Add `additions` and `deletions` to log entry type + - *Impact: Prevents runtime type errors and improves developer experience* + +2. **High Priority - Convert require() to ES import** + - Move `import { spawnSync } from 'child_process';` to file top + - Removes implicit `any` and improves type safety + - *Impact: Better type inference, cleaner code* + +3. **Medium Priority - Extract worktree logic to service class** + - Create `src/main/services/GitWorktreeService.ts` + - Move complex worktree logic from handlers + - Handlers become thin wrappers calling service methods + - *Impact: Improved testability, reduced handler complexity* + +4. **Medium Priority - Extract log parsing to shared utility** + - Add `parseGitLog()` function to `src/shared/gitUtils.ts` + - Consistent with existing pattern (parseGitBranches, parseGitTags, etc.) + - *Impact: Better separation of concerns, reusable parsing* + +5. **Medium Priority - Define shared worktree result types** + - Add to `src/shared/types.ts`: + - `WorktreeInfoResult` + - `WorktreeSetupResult` + - `WorktreeCheckoutResult` + - Use in both handler and preload.ts + - *Impact: Type consistency, reduced duplication* + +6. **Low Priority - Standardize handler wrapper usage** + - Choose between `withIpcErrorLogging` and `createIpcHandler` consistently + - Or document when to use each + - *Impact: Code consistency, easier maintenance* + +7. **Low Priority - Extract magic strings to constants** + - Create `GIT_COMMANDS` or similar constant object + - Extract buffer size to named constant + - *Impact: Easier maintenance, clearer intent* + +## Estimated Impact + +**Risk Level: Low-Medium** + +The git handlers are critical for session state and UI updates, but: +- Changes are mostly type-level and shouldn't affect runtime behavior +- Worktree operations are used for Auto Run parallelization, requires careful testing +- Return type fixes in preload.ts may require updates in renderer code (GitStatusContext, etc.) + +**Testing Considerations:** +- Integration tests needed for worktree operations +- Verify git status/diff return type changes don't break renderer consumers +- Test git log parsing with various commit formats +- Test binary file handling (showFile) with large images