refactor: added agentStore for agent related state management. useAgentListeners hook has replaced IPC listener code in App.tsx#341
Conversation
…etection cache Create agentStore as an orchestration layer over sessionStore + IPC, following the tabStore pattern. Extract 5 error recovery callbacks (clearAgentError, startNewSession, retry, restart, authenticate) from App.tsx into agentStore actions. App.tsx retains thin wrappers for UI side-effects (focus input, close modal). - Create agentStore with agent detection cache (refreshAgents, getAgentConfig) - Create error recovery actions that compose sessionStore mutations + IPC - Create killAgent/interruptAgent lifecycle actions - Wire App.tsx error recovery to agentStore (34 new tests, 18,618 total)
Add comprehensive edge cases for all agentStore actions: retry/restart order verification, authenticate idempotency, killAgent/interruptAgent isolation, action stability for all 9 actions, getAgentActions end-to-end verification, store reset, and concurrent recovery scenarios.
…pp.tsx Phase 2 — useAgentListeners hook: - Extract IPC listener useEffect (~1,520 lines) from App.tsx into src/renderer/hooks/agent/useAgentListeners.ts - Covers onData, onUsage, onStderr, onExit, onAgentError, onThinkingChunk, onToolExecution, onCommandExit, onSlashCommands, onSessionId, onSshRemote - 29 tests for listener registration and cleanup Phase 3 — processQueuedItem + addLogToTab: - Move processQueuedItem (~280 lines) from App.tsx to agentStore action - Move addLogToTab from App.tsx useCallback to sessionStore action - App.tsx delegates via thin wrappers to store actions - 48 new agentStore tests (82 total), 8 new sessionStore tests (88 total) Hooks directory cleanup: - useInlineWizard.ts → hooks/batch/useInlineWizard.ts - useStats.ts → hooks/stats/useStats.ts - Update barrel exports and ~90 import paths Defensive fixes (main process): - Strip Electron/IDE env vars from child processes (envBuilder.ts) - Add stdin error handler in ChildProcessSpawner.ts Dead code removal across ~60 components (unused React imports, unused variables, unreachable code paths in graphDataBuilder/DocumentEditor). App.tsx: 14,500 → 12,650 lines. All 18,714 tests pass.
Pull Request Review: agentStore + App.tsx RefactoringOverviewThis is an excellent refactoring that significantly improves the codebase architecture. The extraction of ~1,850 lines from App.tsx into dedicated stores and hooks is a major win for maintainability. Strengths1. Architecture & Design ✅
2. Code Quality ✅
3. Critical Defensive Fixes ✅
4. Organization ✅
Observations & Recommendations1. Potential Race Condition in processQueuedItem
|
Add early return guard in processQueuedItem when both tabByItemId and getActiveTab return undefined, preventing spawn with invalid tab ID.
Code Review: PR #341 - Renderer State RefactoringThis is an excellent architectural refactoring that successfully decouples complex agent lifecycle management from App.tsx into well-organized stores and hooks. The code quality is high, test coverage is comprehensive, and the security improvements are well-designed. 📊 Overall AssessmentRecommendation: ✅ APPROVE with minor follow-up items Summary:
🎯 Key Achievements1. agentStore Introduction ✅The new Zustand store (534 lines) centralizes agent detection cache, error recovery, and queue processing. Follows the established tabStore pattern correctly. Strengths:
2. useAgentListeners Hook Extraction ✅Successfully extracts ~1,520 lines of IPC listener logic from App.tsx into a dedicated hook (1,582 lines). Strengths:
3. Security Improvements ✅Environment variable sanitization and defensive stdin error handling. Strengths:
4. Hooks Reorganization ✅Logical domain-based directory structure. 🐛 Issues FoundHIGH Priority1. Session Stuck in Busy State on Error (agentStore.ts:470-476) Problem: If all tabs are deleted concurrently during queue processing, error handler returns unmodified session, leaving it stuck in busy state. Impact: Broken session state, user cannot interact with session MEDIUM Priority2. Tab Deletion Race Condition (agentStore.ts:239-242) Problem: If item.tabId tab is deleted after queueing but before processing, command executes on wrong tab. Impact: Commands execute on wrong tabs in edge cases 3. Duplicate Stdin Error Handlers (ChildProcessSpawner.ts:348-365, 444-455) Problem: Both handlers fire for same error with inconsistent logging LOW Priority4. Multiple setSessions() Calls (useAgentListeners.ts) 5. Type Casting to any (useAgentListeners.ts:82, 120, 422) 💡 RecommendationsMust Fix Before Merge
Should Fix (Post-Merge OK)
Nice to Have
🧪 Test Coverage: Excellent ✅
🔒 Security: Well-Designed ✅
📝 Code Quality Score: 8.7/10
✅ ConclusionThis PR represents high-quality architectural work that significantly improves code organization and maintainability. The single HIGH priority issue should be fixed before merge (straightforward fix). All other issues are minor polish items for post-merge. Great work on this refactoring! 🎉 |
…te handlers, type safety - Always reset session to idle in processQueuedItem error path, even when no active tab exists - Abort queue processing when target tab was deleted after queueing instead of executing on wrong tab - Remove duplicate stdin error handler in ChildProcessSpawner (consolidate to single handler) - Replace `any` type casts in useAgentListeners deps with proper types (Toast, HistoryEntryInput, GlobalStats, Group)
Code Review: Agent Store RefactoringThis is an excellent architectural refactoring that significantly improves the codebase organization. The PR successfully extracts agent-related state management from App.tsx into dedicated stores and hooks following established patterns. Here's my detailed feedback: ✅ Strengths1. Architectural Improvements
2. Code Quality
3. Testing
4. Performance Considerations
5. Security Improvement (envBuilder.ts)
🔍 Issues & Concerns1. Critical: SSH Remote Execution AwarenessLocation: agentStore.ts:processQueuedItem (lines 232-507) The processQueuedItem function spawns agent processes but does NOT explicitly wrap with SSH when session.sessionSshRemoteConfig?.enabled is true. According to CLAUDE.md:
Current approach (line 338): Verification needed:
Impact: If SSH is not handled by the spawn API, queue processing will always execute locally even when configured for SSH remote, breaking user expectations. Recommendation: Verify with the team that sessionSshRemoteConfig parameter is sufficient, or add explicit SSH wrapping. 2. Potential Bug: Tab Deletion Race ConditionLocation: agentStore.ts:processQueuedItem (lines 240-260) Good defensive check for deleted tabs, but if the queue has multiple items and we abort here, those remaining items are silently left in the queue. Recommendation: Consider clearing the execution queue on abort: return {
...s,
state: 'idle' as SessionState,
busySource: undefined,
thinkingStartTime: undefined,
executionQueue: [], // Clear queue since target tab was deleted
};Or add a comment explaining why preserving the queue is intentional. 3. Code Smell: Large Action FunctionLocation: agentStore.ts:processQueuedItem (lines 232-507) This single action is ~280 lines with deeply nested logic for tab resolution, agent detection, read-only filtering, message/command handling, template substitution, and error handling. Impact: Hard to test individual pieces, hard to modify without introducing bugs. Recommendation: Extract sub-functions (not blocking for this PR):
4. Minor: Inconsistent Error LoggingLocation: agentStore.ts (multiple) Some functions log errors to console, others throw, some do both. Examples:
Recommendation: Standardize error handling strategy per function type:
Current behavior is actually reasonable - this is a minor note for future consistency. 5. Documentation Gap: ProcessQueuedItemDepsLocation: agentStore.ts (lines 111-119) The dependency object is well-typed but lacks usage documentation. Why are these passed as dependencies instead of being fetched inside the action? Recommendation: Add a comment explaining: /**
* Dependencies passed from App.tsx for processQueuedItem.
* These come from hooks/state that cannot be imported at module level:
* - conductorProfile: From useConductor hook (changes at runtime)
* - custom/speckit/openspec commands: From settings hooks (dynamic config)
*/🎯 RecommendationsMust Verify Before Merge
Should Fix Before Merge
Nice to Have (Follow-up PR)
📊 Performance & SecurityPerformance: ✅ Excellent
Security: ✅ Improved
🧪 Test CoverageCurrent: 18,714 tests passing, 0 TypeScript errors Test breakdown:
Recommendation: Add integration test for SSH remote execution path once behavior is verified. ✅ Overall AssessmentThis is a high-quality refactoring that significantly improves the codebase architecture. The extraction follows established patterns, maintains excellent test coverage, and includes important security fixes. Key achievements:
Areas for attention:
Approval Status: ✅ Approve with minor comments The concerns raised are either verification items (SSH handling) or minor improvements that can be addressed in follow-up PRs. The comprehensive testing (18,714 tests passing) gives confidence that the refactoring is solid. Excellent work! This refactoring will make future feature development much easier and the codebase significantly more maintainable. 🎉 📝 Additional NotesFor future refactoring PRs:
Specific files to watch:
|
1 participant
Renderer State Refactoring: agentStore + App.tsx Extraction
Summary
agentStore(Zustand) to centralize agent detection cache, error recovery actions, and process queue management — replacing scattered App.tsx local state and callbacksuseAgentListenershook (~1,520 lines) from App.tsx, moving all IPC listener setup (onData, onUsage, onStderr, onExit, onAgentError, onThinkingChunk, onToolExecution, etc.) into a dedicated hook underhooks/agent/processQueuedItem(~280 lines) from App.tsx into an agentStore action, decoupling queue processing from the React component treeaddLogToTabfrom App.tsxuseCallbackto a sessionStore action for consistent log managementSTRIPPED_ENV_VARSinenvBuilder.ts) and add stdin error handler inChildProcessSpawner.tsto prevent silent EPIPE failuresuseInlineWizard→batch/,useStats→stats/)Key changes
stores/agentStore.tsprocessQueuedItemactionhooks/agent/useAgentListeners.tsstores/sessionStore.tsaddLogToTabactionApp.tsxenvBuilder.tsELECTRON_RUN_AS_NODE,CLAUDECODE,NODE_ENV, etc. from spawned agent environmentsChildProcessSpawner.tshooks/batch/,hooks/stats/hooks/into domain subdirectoriesTest coverage
processQueuedItem)addLogToTabTest plan
npx tsc --noEmit --pretty— 0 errorsnpm run test— 18,714 tests pass